Skip to content

Bugfix/article ID parsing issue#6

Closed
KevinZonda wants to merge 1 commit intojannisborn:mainfrom
KevinZonda:patch-1
Closed

Bugfix/article ID parsing issue#6
KevinZonda wants to merge 1 commit intojannisborn:mainfrom
KevinZonda:patch-1

Conversation

@KevinZonda
Copy link

@jannisborn
Copy link
Owner

Hi @KevinZonda,
Thanks for your interest and this PR. You need to provide a bit more context here, ideally a test or MWE that fails with the old version and works with your fix. E.g., you're pointing to gijswobben/pymed#30 which updates article.py but your change is in book.py. When testing your code with the original query from the example, nothing changes, so I dont see any reason to merge the PR as is.

Also gijswobben/pymed#31 provides an alternative solution to the original issue (gijswobben/pymed#22) and we need to understand which is better.

We can fix the original issue in this library, but if we do it, we do it once and in a clean way. Thanks for your support!

Copy link
Owner

@jannisborn jannisborn left a comment

Choose a reason for hiding this comment

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

See above comment

@jannisborn
Copy link
Owner

@KevinZonda I opened #7 to fix the issue you mention by changing article.py (rather than book.py). Let me know if you have any thoughts on this, otherwise #7 will be merged soon instead of this PR.
Thx for raising this issue

@jannisborn jannisborn added the invalid This doesn't seem right label May 5, 2025
@jannisborn
Copy link
Owner

Superseded by #7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

invalid This doesn't seem right

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants