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

Add Bioconductor translator #1983

Merged
merged 18 commits into from Sep 17, 2019
Merged

Add Bioconductor translator #1983

merged 18 commits into from Sep 17, 2019

Conversation

hubentu
Copy link
Contributor

@hubentu hubentu commented Jul 31, 2019

The search page ("multiple") worked manually but failed in the testing. Could anyone help to debug? Thanks!

@hubentu
Copy link
Contributor Author

hubentu commented Aug 1, 2019

I have no idea about the error. Could anyone help to check? Thanks! @zuphilip

@zuphilip
Copy link
Contributor

zuphilip commented Aug 7, 2019

This are "only" linting issues which can also be fixed later. It is not an error per se.

However, try to change the line

return true;

into simply return; which jumps out of the function without a return value, which otherwise is not given in all other cases.

@hubentu
Copy link
Contributor Author

hubentu commented Aug 7, 2019

@zuphilip Thank you! Finally, all checks have passed. Hope it can be accepted soon :)

@zuphilip zuphilip added the New Translator Pull requests for new translators label Aug 7, 2019
@zuphilip
Copy link
Contributor

zuphilip commented Aug 7, 2019

Great! I can't promise to review it fast...

Copy link
Contributor

@zuphilip zuphilip left a comment

Choose a reason for hiding this comment

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

Thank you for this PR! The code already looks quite good and most things are stable even while the website itself does not have much structure to extract the information from. There are some issues to fix and improve, see my review, but everything should be doable. Let us when you are through, then we can check it again and merge it.

Bioconductor.js Outdated Show resolved Hide resolved
Bioconductor.js Outdated Show resolved Hide resolved
Bioconductor.js Outdated Show resolved Hide resolved
Bioconductor.js Show resolved Hide resolved
Bioconductor.js Outdated Show resolved Hide resolved
Bioconductor.js Show resolved Hide resolved
Bioconductor.js Outdated Show resolved Hide resolved
Bioconductor.js Outdated Show resolved Hide resolved
Bioconductor.js Outdated Show resolved Hide resolved
Bioconductor.js Show resolved Hide resolved
Bioconductor.js Outdated Show resolved Hide resolved
Bioconductor.js Outdated Show resolved Hide resolved
Copy link
Contributor

@zuphilip zuphilip left a comment

Choose a reason for hiding this comment

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

This looks fine in general, but there are some small issues to fix. Please ping me again if you are through.

@zuphilip zuphilip merged commit 740ef58 into zotero:master Sep 17, 2019
@zuphilip
Copy link
Contributor

Merged now! 🚀 Thank you very much @hubentu for all the work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
New Translator Pull requests for new translators
Development

Successfully merging this pull request may close these issues.

None yet

2 participants