-
-
Notifications
You must be signed in to change notification settings - Fork 338
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
Added e2e test for converting a text column of numbers to number column fixes #1128 #1170
Added e2e test for converting a text column of numbers to number column fixes #1128 #1170
Conversation
of numbers to number column.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ManishShah120 Thanks for the PR! Looks good mostly, but I have a few comments which need to be resolved before we can merge in the changes.
Codecov Report
@@ Coverage Diff @@
## master #1170 +/- ##
=======================================
Coverage 93.39% 93.39%
=======================================
Files 113 113
Lines 4332 4332
=======================================
Hits 4046 4046
Misses 286 286
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
@pavish please have a look at it, if its ok to merge or if any changes is/are required. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ManishShah120 The PR looks good to me.
I do have a couple suggestions to improve readability and re-usability of code.
- The fixture
go_to_table_with_numbers_in_text
can be made more common and provide a table which allows type conversions from text to all types (eg., text -> boolean, text -> uri etc.,), instead of being specific to 'text -> number'. - The naming of column can be improved. Instead of
foo
, the column name could betext_column_with_numbers
.
I'm not going to block this PR for these comments, but please feel free to raise another PR with these changes.
I'm approving it. I'll leave it to @mathemancer to take a look at the python code and approve as the backend reviewer.
|
||
|
||
@pytest.fixture | ||
def go_to_table_with_numbers_in_text(page, patent_schema, create_column, schema, base_schema_url): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def go_to_table_with_numbers_in_text(page, patent_schema, create_column, schema, base_schema_url):
In this line it will be more appropriate to use custom_types_schema
instead of patent_schema
, once the PR #1199 is merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks good, subject to @pavish 's suggestion about the scope of the test table. Thank you for your effort on this PR!
Fixes #1128
Added an e2e test for converting a text column of numbers to number column.
Technical details
Screenshots
Checklist
Update index.md
).master
branch of the repositoryvisible errors.
Developer Certificate of Origin
Developer Certificate of Origin