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

Add dummy sheet test #28

Merged
merged 38 commits into from
Dec 10, 2021
Merged

Add dummy sheet test #28

merged 38 commits into from
Dec 10, 2021

Conversation

gaurav
Copy link
Collaborator

@gaurav gaurav commented Dec 7, 2021

This PR adds a "dummy sheet test" (#8) -- it runs sheet2linkml with a Google Sheet I've created solely for testing at https://docs.google.com/spreadsheets/d/1HpsF12vl_CeIzV2TEtANeysnh7cy6YOUDdv6fMXT00I/edit#gid=1810442705, and ensures that the output is identical to the schema generated on previous runs, which is included in this PR.

In addition to this, this PR also fixes some minor bugs found along with the way:

  • The CCDH Terminology Service enumeration endpoint changed its format, so we have to modify our API to match it.
  • The CCDH Terminology Service now returns 404s if the field doesn't exist or is not an enumeration, so we modify our code to catch that possibility.
  • Fixes Update google-sheets to support Google API authentication via environmental variable #10 by allowing the use of Google API Service Accounts in an environmental variable, allowing testing on GitHub Actions.
  • Added pytest to support testing.
  • Added a command line option to set the Google Sheet ID.
  • Added checks to ensure that the logging_config and google_sheet_id command line arguments are set before using them.
  • Made sheet2linkml explicitly set the error code to 0 in case of successful execution.
  • Replaced a stray print() with logging.debug().

@gaurav gaurav requested a review from sujaypatil96 December 7, 2021 07:36
@gaurav
Copy link
Collaborator Author

gaurav commented Dec 7, 2021

@turbomam @jooho-lee-kim You don't need to review this PR (unless you want to!), but I just wanted to let you both know that this test now exists. Let me know if you have any ideas on improving it, either in this PR or in another one!

@gaurav gaurav removed the request for review from sujaypatil96 December 9, 2021 20:22
@gaurav gaurav marked this pull request as draft December 9, 2021 20:22
@gaurav gaurav marked this pull request as ready for review December 9, 2021 22:25
@gaurav gaurav force-pushed the add-dummy-sheet-test branch from 349df54 to f431144 Compare December 9, 2021 22:36
@gaurav gaurav force-pushed the add-dummy-sheet-test branch from f431144 to 2870a01 Compare December 9, 2021 22:38
@gaurav gaurav requested a review from sujaypatil96 December 9, 2021 23:27
Copy link
Collaborator

@sujaypatil96 sujaypatil96 left a comment

Choose a reason for hiding this comment

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

All the changes and tests in this PR are great @gaurav. Awesome job.🎉

I've left some very very trivial comments on this PR, which can choose not to address, they're not critical at all. So approving this PR for merge.🚀

.github/workflows/pytest.yaml Outdated Show resolved Hide resolved
sheet2linkml/cli.py Show resolved Hide resolved
sheet2linkml/cli.py Outdated Show resolved Hide resolved
sheet2linkml/cli.py Outdated Show resolved Hide resolved
sheet2linkml/cli.py Outdated Show resolved Hide resolved
sheet2linkml/cli.py Outdated Show resolved Hide resolved
sheet2linkml/terminologies/tccm/api.py Show resolved Hide resolved
tests/test_command_line.py Show resolved Hide resolved
@gaurav gaurav merged commit b23a539 into develop Dec 10, 2021
@gaurav gaurav deleted the add-dummy-sheet-test branch December 10, 2021 06:10
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.

Update google-sheets to support Google API authentication via environmental variable
2 participants