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

WIP: Added Qiita study reserved-word check #91

Merged
merged 2 commits into from
Sep 12, 2024

Conversation

charles-cowart
Copy link
Contributor

Depends on an additional PR to mg-scripts before passing tests.

Copy link
Member

@antgonza antgonza left a comment

Choose a reason for hiding this comment

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

One comment.

Comment on lines +1004 to +1008
fake_client.info_in_11661['categories'].append('well_id_384')
fake_client.info_in_13059['categories'].append('well_id_384')

msg = ("'well_id_384' exists in Qiita study 13059's sample metadata"
"\n'well_id_384' exists in Qiita study 11661's sample metadata")
Copy link
Member

Choose a reason for hiding this comment

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

My concern with this is that we are not actually testing the connection to qiita as you are using the fake_client, would it be possible to create a real one? Note that all plugins use this. See: https://github.com/qiita-spots/qp-woltka/blob/main/qp_woltka/tests/test_woltka.py#L26

Copy link
Member

Choose a reason for hiding this comment

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

After discussion offline, @charles-cowart confirmed that he's pointing to the right endpoint cause he tested outside of the tests + we agreed that reintroducing actual testing against the local/dev version will be in a different PR.

@antgonza antgonza merged commit 1cd30c4 into qiita-spots:main Sep 12, 2024
2 checks passed
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.

Confirm Qiita Study sample-templates do not already contain well_id_384 and other reserved columns.
2 participants