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

Implement method for mobile-html #17

Merged
merged 6 commits into from
Apr 22, 2021

Conversation

friendofdog
Copy link
Contributor

Takes a page title and returns mobile-optimised HTML.

closes #14

Had previously been calling .json() on all responses, even if they
were strings. Can now handle JSON and string responses.

resolving dopecodez#14
Returns HTML for mobile for a given page.

resolving dopecodez#14
@friendofdog
Copy link
Contributor Author

I made an attempt at this issue but ran into some issues with makeRestRequest(). This function calls .json() on all responses, which will not work if the response is a string (like an HTML document). I changed makeRestRequest() in ba806cc so it can handle strings (all tests still pass) but I'm not sure how this might impact other parts of the app.

@dopecodez
Copy link
Owner

dopecodez commented Apr 19, 2021

I checked out the other functions of your forks quickly and it looks like everything is working. I'll check out the resultTypes and options and merge this tomorrow.

Feel free to change this from a draft when you are good with the changes.

@friendofdog friendofdog marked this pull request as ready for review April 19, 2021 22:55
@friendofdog
Copy link
Contributor Author

Thanks for the feedback. I just added mobileHtml() to docs/wiki.md and un-drafted the PR.

@dopecodez
Copy link
Owner

dopecodez commented Apr 20, 2021

I renamed the test file to match jest naming conventions and also added an extra test. I think the changes are looking good. We'll merge them once you have a chance to look through my changes.

I also added mobileHtml to the page and added a few tests.

@friendofdog
Copy link
Contributor Author

I renamed the test file to match jest naming conventions and also added an extra test. I think the changes are looking good. We'll merge them once you have a chance to look through my changes.

I also added mobileHtml to the page and added a few tests.

There's actually no error thrown if the page isn't found. Wikipedia returns the below object, so I just passed it to the response.

Should I instead do something like throw new htmlError("Page not found"); if the Wikipedia API returns the "not found" object?

{
  type: 'https://mediawiki.org/wiki/HyperSwitch/errors/not_found',
  title: 'Not found.',
  method: 'get',
  detail: 'Page or revision not found.',
  uri: '/en.wikipedia.org/v1/page/mobile-html/Qqqqqqqq'
}

@dopecodez
Copy link
Owner

No @friendofdog , i think its fine for now every REST method is implemented that way.

We'll let it handle other errors and the Not Found can be returned like it is handled currently.

@friendofdog
Copy link
Contributor Author

@dopecodez understood. This PR is okay to merge now?

@dopecodez
Copy link
Owner

Thanks for the PR @friendofdog , your work keeps this repo moving.

We should definitely do something about the coverage failing like discussed in https://github.community/t/make-secrets-available-to-builds-of-forks/16166. I'll open a separate issue for this as this makes PRs look bad.

@dopecodez dopecodez merged commit 8c0db7e into dopecodez:master Apr 22, 2021
@friendofdog friendofdog deleted the mobile-html branch January 8, 2022 05:33
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.

Implement mobile html
2 participants