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

Fix undefined behaviour for pages with numbers as title (e.g 7) #52

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

phiph-s
Copy link

@phiph-s phiph-s commented Jul 2, 2023

When requesting the page "23", it is expected to receive this page: https://en.wikipedia.org/wiki/23
Expected behavior:
wiki.page("23") -> https://en.wikipedia.org/wiki/23
wiki.page(23) -> https://en.wikipedia.org/wiki/Assistive_technology

Actual behavior:
wiki.page("23") -> https://en.wikipedia.org/wiki/Assistive_technology (which has ID 23)
wiki.page(23) -> https://en.wikipedia.org/wiki/Assistive_technology

So there is no way to get the page for the number 23 or for example years like 1939, sine they will be treated as IDs.

It is checked whether an ID or a Title is given by isNaN(title) in utils.ts, the String "7" will be handled like a page ID.
Instead check the actual type of what the function receives:
return typeof title === 'string' || title instanceof String;

New behavior:
wiki.page("23") -> https://en.wikipedia.org/wiki/23
wiki.page(23) -> https://en.wikipedia.org/wiki/Assistive_technology

-> Supply a number, get a page by ID, supply a string, get the page by title

@phiph-s
Copy link
Author

phiph-s commented Jul 2, 2023

Accidentally added a version change to the pull request... My bad

@phiph-s
Copy link
Author

phiph-s commented Jul 3, 2023

I saw in the Tests that you actually test against strings as IDs. If the behavior is wanted like this, I would at least consider adding parameters to the query that define whether the string should be treated as ID or title.

@dopecodez
Copy link
Owner

Its a very interesting point @phiph-s - apologies for the late reply i was travelling last week.

You are correct in saying that currently we do consider numbers as ids particularly. Your MR makes sense to me but i think we need some more test cases. I will work on the same in the coming weekend if you are busy with something else.

Thanks for the MR @phiph-s

@phiph-s
Copy link
Author

phiph-s commented Jul 13, 2023

I'm currently quite limited in time, but maybe I'm able to write some test cases.
Another option would be to implement the 2 functions "pageById" and "pageByTitle", in order to not change the behavior of the current implementation of "page", as this change might break some projects that use this package.

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.

2 participants