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

IMP: more verbose error handling for merge method use cases #64

Merged
merged 11 commits into from
May 1, 2024

Conversation

hagenjp
Copy link
Contributor

@hagenjp hagenjp commented Apr 9, 2024

closes #63
closes #62

@hagenjp hagenjp assigned hagenjp and unassigned hagenjp Apr 11, 2024
@hagenjp hagenjp requested a review from lizgehret April 11, 2024 20:56
Copy link
Member

@lizgehret lizgehret left a comment

Choose a reason for hiding this comment

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

Thanks for tackling this one @hagenjp! Some comments inline, lmk if you have any questions!

q2_metadata/_merge.py Outdated Show resolved Hide resolved
q2_metadata/_merge.py Outdated Show resolved Hide resolved
q2_metadata/_merge.py Outdated Show resolved Hide resolved
q2_metadata/tests/test_merge.py Outdated Show resolved Hide resolved
q2_metadata/tests/test_merge.py Outdated Show resolved Hide resolved
q2_metadata/_merge.py Outdated Show resolved Hide resolved
@hagenjp hagenjp requested a review from lizgehret April 24, 2024 17:42
Copy link
Member

@lizgehret lizgehret left a comment

Choose a reason for hiding this comment

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

One very small nit-picky update to your Regex assertion (noted below) but otherwise this all looks great!

q2_metadata/tests/test_merge.py Show resolved Hide resolved
q2_metadata/tests/test_merge.py Show resolved Hide resolved
@hagenjp hagenjp requested a review from lizgehret April 30, 2024 16:19
@lizgehret lizgehret assigned lizgehret and unassigned hagenjp May 1, 2024
Copy link
Member

@lizgehret lizgehret left a comment

Choose a reason for hiding this comment

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

thanks for all of your hard work on this @hagenjp, this is g2g! i replaced the parens around the MD column types with single quotes in the error message, just because i think it renders a bit nicer - but didn't think that warranted another round of reviews and didn't want to toss it back to you for something that small. once this passes ci, i'll go ahead and merge!

@lizgehret lizgehret removed their assignment May 1, 2024
@lizgehret lizgehret merged commit 2766cad into qiime2:dev May 1, 2024
4 checks passed
@lizgehret lizgehret changed the title IMP: Error handling for merge method use cases IMP: more verbose error handling for merge method use cases May 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Completed
3 participants