Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dataset Management API Support #83

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

PeterAustinMoore
Copy link

This is my first foray into C# and .NET, so any pointers/feedback greatly appreciated. I tried to set up the building blocks based off of https://github.com/socrata/socrata-py

What it adds:

  • CreateDataset method through DSMAPI
  • CreateRevision off of a currently existing dataset
  • Error Rows for catching problematic data

What it removes:

  • Nothing

What it could do in the future:

  • Column level schema changes on dataset creation/revision
  • Using Config (similar to socrata-py)
  • Metadata only dataset updates
  • Create revision based off external link, the current dataset, or a Gateway

@JoeNunnelley
Copy link

General : Run FxCop on this code base (or some more modern analog)
https://docs.microsoft.com/en-us/previous-versions/dotnet/netframework-3.0/bb429476(v=vs.80)?redirectedfrom=MSDN
also run
https://github.com/StyleCop

File : SODA.Tests/SodcaClientTests.cs

  • From the readme it looks like we are renaming DSMAPI intannces to SODAClient. Here we are naming tests with DSMAPI in them and I wonder if we should convert to using SODAClient throughout?
    File : SODA/SodaClient.cs
  • Some instances where you use str == null to check values when its probably ok to use the IsNullOrEmpty() function as you do in other locations

File : README.md

  • you can use this formulation to avoid all the extra escape chars in your example code : string path1 = @"c:\temp\MyTest.txt";

File : SODA/PipelineJob.cs

  • public string Username => publis string username to match other variable use in the files.

Copy link

@JoeNunnelley JoeNunnelley left a comment

Choose a reason for hiding this comment

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

here are some suggestions. probably the most valuable will be to run static code analysis tools and style tools on this project.

SODA/PipelineJob.cs Outdated Show resolved Hide resolved
SODA/PipelineJob.cs Outdated Show resolved Hide resolved
SODA/SchemaTransforms.cs Outdated Show resolved Hide resolved
/// <exception cref="System.ArgumentOutOfRangeException">Thrown if the specified <paramref name="resourceId"/> does not match the Socrata 4x4 pattern.</exception>
public Revision CreateRevision(string method, string resourceId, string permission = "private")
{

Choose a reason for hiding this comment

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

extra line

SODA/SodaClient.cs Show resolved Hide resolved
SODA/Utilities/SodaUri.cs Outdated Show resolved Hide resolved
@thekaveman
Copy link
Contributor

@PeterAustinMoore this is a tremendous effort, thank you for submitting the PR!

I haven't had the time to fully dig in and review yet, but I wanted to let you know its on our radar. One thing that would greatly help the review is if you could rebase your branch on master (we had some in-flight changes that were just released yesterday).

I can't promise when we'll get to this, but we are definitely interested in folding in the functionality. Stay tuned!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants