Skip to content

Conversation

eerhardt
Copy link
Member

Renaming the namespace of the IDataView types.

Follow up to #1860

I also added a few copyrights where they were missing.

@codecov
Copy link

codecov bot commented Jan 26, 2019

Codecov Report

Merging #2254 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           master   #2254   +/-   ##
======================================
  Coverage    80.8%   80.8%           
======================================
  Files         159     159           
  Lines       28503   28503           
  Branches     1910    1910           
======================================
  Hits        23031   23031           
  Misses       5175    5175           
  Partials      297     297
Flag Coverage Δ
#Debug 80.8% <ø> (ø) ⬆️
#test 80.8% <ø> (ø) ⬆️

@@ -18,6 +19,7 @@
using Parquet;
using Parquet.Data;
using Parquet.File.Values.Primitives;
using DataViewSchema = Microsoft.Data.DataView.Schema;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea was broached I know, but resolution never reached: should we rename? Or else perhaps make Schema a nested class of something appropriate, so as to give it definition?

I guess if something as useful as the Parquet format can get away with naming something "schema" without complaint, maybe we (being, I suppose, less universally beloved for now) can get away with it as well fairly easily.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could rename the class to DataViewSchema, or ViewSchema.

We’d probably want to rename everything then. Row is a pretty generic name. So is ColumnType.

Copy link
Contributor

@TomFinley TomFinley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for doing this @eerhardt ... out of curiosity, I hope you did such a massive change via automation, and if you did, may I know what tool you used?

@eerhardt
Copy link
Member Author

may I know what tool you used?

The Error List Window in VS, and CTRL+. -> Add Using.

:(

diagPs.CreateDiagnosticResult(37, 13, "Class8", "F2"),
diagPg.CreateDiagnosticResult(38, 13, "Class9", "F2"),
diagPc.CreateDiagnosticResult(39, 13, "Class10"),
diagPc.CreateDiagnosticResult(40, 13, "Class11"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was scrolling thru, getting carpal tunnel, about to conclude "oh look an easy PR approval".

What are these changing in response to?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is our C# diagnostics analyzer test, which ensures our code analyzer gives the right warnings/errors on the correct code lines.

Because the code being analyzed used an ‘IDataView’ variable, I needed to insert a new ‘using’ statement at the top of the code. This moved all the code down one line, causing the tests to now be off by one line.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems a bit hard to maintain.

@eerhardt eerhardt merged commit 7fc7e50 into dotnet:master Jan 28, 2019
@eerhardt eerhardt deleted the NamespaceRename branch January 28, 2019 21:00
@ghost ghost locked as resolved and limited conversation to collaborators Mar 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants