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

adding UNQUOTED_SUFFIX to sort_order #190

Merged

Conversation

samshuster
Copy link
Contributor

Summary of Changes

Fixes sort order when using standalone column model.

NOTE: I wonder if standalone column model should be deprecated or refactored so that it doesn't get out of sync with other column model? If primary purpose is for sample data loader, there is a different approach that we can take.

Tests

None

Documentation

None

CheckList

Make sure you have checked all steps below to ensure a timely review.

  • PR title addresses the issue accurately and concisely. Example: "Updates the version of Flask to v1.0.2"
  • PR includes a summary of changes.
  • PR adds unit tests, updates existing unit tests, OR documents why no test additions or modifications are needed.
  • In case of new functionality, my PR adds documentation that describes how to use it.
    • All the public functions and the classes in the PR contain docstrings that explain what it does
  • PR passes make test

@codecov-io
Copy link

codecov-io commented Feb 6, 2020

Codecov Report

Merging #190 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #190      +/-   ##
==========================================
+ Coverage   82.07%   82.08%   +<.01%     
==========================================
  Files          70       70              
  Lines        3381     3382       +1     
  Branches      364      364              
==========================================
+ Hits         2775     2776       +1     
  Misses        499      499              
  Partials      107      107
Impacted Files Coverage Δ
databuilder/models/standalone_column_model.py 79.62% <100%> (+0.38%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1e01e8c...f3f26b7. Read the comment docs.

@feng-tao
Copy link
Member

feng-tao commented Feb 7, 2020

I think it should deprecate IMO as it is requested and used for csv/standalone use case from Shaun.

@samshuster
Copy link
Contributor Author

ok, to get rid of standalone column model, I would just need to add an extractor (that I have already implemented) that properly creates the table model using both columns and tables. I can put this in the sample script section.

If you would like, I can either modify this MR to go in that direction, or I can have that be a separate issue.

@feng-tao
Copy link
Member

feng-tao commented Feb 7, 2020

@samshuster let's merge this one. And have a new pr to remove this model.

@feng-tao feng-tao merged commit a248d40 into amundsen-io:master Feb 7, 2020
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