8000 Bugfix/article ID parsing issue by KevinZonda · Pull Request #6 · jannisborn/pymed · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Bugfix/article ID parsing issue #6

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Bugfix/article ID parsing issue #6

wants to merge 1 commit into from

Conversation

KevinZonda
Copy link

@jannisborn
Copy link
8000
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!

jannisborn
jannisborn requested changes Apr 27, 2025
View reviewed changes
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

@jannisborn jannisborn closed this May 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jannisborn jannisborn jannisborn requested changes

Assignees
No one assigned
Labels
invalid This doesn't seem right
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@KevinZonda @jannisborn
0