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 support for Kluwer Arbitration #3161

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

Conversation

thebluepotato
Copy link
Contributor

Very alpha for now, but adds support for articles and books from Kluwer Arbitration. I should probably add blog posts at some point.

@thebluepotato thebluepotato marked this pull request as draft October 18, 2023 10:05
@thebluepotato thebluepotato marked this pull request as ready for review October 19, 2023 23:05
@thebluepotato
Copy link
Contributor Author

thebluepotato commented Oct 19, 2023

The translator works for most documents found on Kluwer Arbitration, but not all options have been explored yet. In particular, I have not been able to run any tests within Scaffold because the JSON requests return null. I thought it was a VPN issues but the integrated browser follows redirections and displays the full document.

EDIT: apparently Scaffold does not use cookies correctly in Zotero 6, was able to use Zotero 7 to generate the test cases. Search is still WIP

@thebluepotato
Copy link
Contributor Author

Ready for production IMO, however the linter seems to complain about the Nullish coalescing assignment (??=) on line 278...

@zoe-translates
Copy link
Collaborator

Hello, thank you for the contribution!

I'd be more than happy to test and review, but unfortunately I don't have access to the Kluwer Arbitration. So all I can do for now is to provide some comments about the coding itself.

Nullish coalescing assignment: This is an ES2020 feature, and the repo's ESLint config is set to ES2018. I don't think any other translator uses ?. and ?? yet, due to compatibility concerns.

@zoe-translates
Copy link
Collaborator

In fact, with the ?. and ?? in the code, ESLint fails to process the whole file at all. If you switch to the pre-ES2020 syntax, you'll find more lint warnings (missing semicolon etc.)

Followed most of the review points, left `async` for now until there's more opinions on this. Will update again when remaining points have been clarified
Better handles the case where the ISBN search failed and relying on Kluwer itself as a fallback.
Copy link
Collaborator

@zoe-translates zoe-translates left a comment

Choose a reason for hiding this comment

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

Hi! Thank you for being with us :)

As I mentioned earlier, I can't help you with live testing, so again here are a few generic comments.

Kluwer Arbitration.js Outdated Show resolved Hide resolved
Kluwer Arbitration.js Outdated Show resolved Hide resolved
Kluwer Arbitration.js Outdated Show resolved Hide resolved
Kluwer Arbitration.js Outdated Show resolved Hide resolved
Refactored search logic and arbitration blog logic (supports multiples blog articles as well). All items set their URL now. Snapshots were considered but rejected since they only offer marginal improvements at a considerably larger size (footnote callout number are not clickable, only improvement is that in the endnotes the numbers are clickable).
@thebluepotato
Copy link
Contributor Author

Thank you so much @zoe-translates and @dstillman for your reviews! If all is ok now, when can we expect the PR to be merged?

Kluwer Arbitration.js Outdated Show resolved Hide resolved
… cases

- Saving dates in ISO format
- When `date` was set for cases or statutes published in a publication, `dateDecided` and `dateEnacted` respectively were ignored
@AbeJellinek
Copy link
Member

I'd like to get this merged! Unfortunately I have no access to the site at all. @thebluepotato, could you verify that tests all still pass for you?

@thebluepotato
Copy link
Contributor Author

Hi! I updated some of the code and tests, and they all passed successfully! Thank you so much for looking at this.

While I have you here, could you look at #3169 as well? I'll be checking if the tests still work, but this is publicly accessible anyway.

@thebluepotato
Copy link
Contributor Author

Btw I think something might be wrong with your CI code as the checks seem to fail despite me running the linter locally

@thebluepotato
Copy link
Contributor Author

@AbeJellinek This PR is also ready for re-review

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

Successfully merging this pull request may close these issues.

4 participants