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

WorldCat Discovery Service.js -- fix eslint warnings #1847

Merged
merged 1 commit into from Feb 25, 2019

Conversation

@retorquere
Copy link
Contributor

commented Feb 25, 2019

No description provided.

@adam3smith adam3smith merged commit 56182d1 into zotero:master Feb 25, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@adam3smith

This comment has been minimized.

Copy link
Collaborator

commented Feb 25, 2019

You're a hero slogging through all of these!

@retorquere retorquere deleted the retorquere:WorldCat_Discovery_Service branch Feb 25, 2019

@retorquere

This comment has been minimized.

Copy link
Contributor Author

commented Feb 25, 2019

I needed something to take my mind off. This is about as Zen as it gets 😄 I have some scripts set up to create branches of course, but cautious checking with git diff -w remains necessary. I''l do a few more tonight but must stop before I get change-blind.

@dstillman

This comment has been minimized.

Copy link
Member

commented Feb 25, 2019

Thanks a lot for doing this, but I would actually hold off before going through more of these. As I noted, we have a much larger ESLint config that we might want to apply to this repo (and most of our repos). I just added that config recently, so there may be some additional tweaks to make (and the warn/error distinction is somewhat arbitrary at this point), but it should mostly describe the bulk of existing Zotero code. Whether that matters for translator is up for debate, but it could certainly address things like this

(I also suggested that we shouldn't do mass edits for style changes and translator authors should just fix them in separate commits when they update styles. Conflicts are much less of an issue here, though, so if @adam3smith and @zuphilip don't mind the extra churn, I'm OK with going through these now. But it doesn't make sense to do it more than once.)

@dstillman

This comment has been minimized.

Copy link
Member

commented Feb 25, 2019

(And one reason to do them along with other translator changes, after expanding the config, is that more intensive checking is likely to reveal actual bugs that need to be investigated and tested against the site.)

@retorquere

This comment has been minimized.

Copy link
Contributor Author

commented Feb 25, 2019

I'm OK holding off, it's mostly a papercuts issue for me. I reckon though that whatever edits I make now are going to have to be done in any case. One argument I could offer for doing it anyhow is that most translators seem to be copy-and-change jobs, so whatever code problems exist are more likely to proliferate.

But I'll stop sending in pull requests for now.

@retorquere

This comment has been minimized.

Copy link
Contributor Author

commented Feb 25, 2019

Those bugs would be unearthed by heavier linting in any case -- if you look at the git diff -w, I'm mostly just changing indents and indexOfs.

But as said, I'll stop for now. Feel free to holler when the new lint config is in place -- code tidying is relaxing work for me right now.

GuyAglionby added a commit to GuyAglionby/translators that referenced this pull request Mar 30, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.