Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign upcleanup BibTeX for eslint rules #1838
Conversation
dstillman
merged commit 58b257f
into
zotero:master
Feb 25, 2019
1 check passed
retorquere
deleted the
retorquere:BibTeX
branch
Feb 25, 2019
dstillman
added a commit
that referenced
this pull request
Mar 6, 2019
This comment has been minimized.
This comment has been minimized.
This contained a bug that resulted in empty imports in some cases. All the translator tests somehow still passed, but it showed up in a Zotero client test after I updated the translators submodule. (Since there was no timestamp change, this didn't get pushed to regular clients, so at most it would only have affected a small number of beta clients that updated in the last hour. I don't usually bother waiting for tests before pushing beta builds after submodule updates, since it's extremely rare for style/translator changes to have any effect.) I merged this, so it's totally on me for not catching the mistake, but this, again, is why I'd like to avoid doing widespread lint-only changes in the absence of other necessary changes to the translator. |
This comment has been minimized.
This comment has been minimized.
I'm all for this - the main reason I want to get the lint sweep out of the way is because the problems it does unearth become more visible. But I've so far done manual fix, not auto-fix, so while I'm doing the manual fixes I generally try to stay vigilant for other bugs in the code I'm going through. I'm with you 100% that linting is not a debugging strategy, but in my experience, visually clean code is easier to debug to begin with, and lint messages like undeclared or shadowed variables in my experience usually are a frequent source of genuine bugs. |
This comment has been minimized.
This comment has been minimized.
BTW I'd be happy to fix that bug that was introduced by this lint sweep (that's what you're saying right?) |
This comment has been minimized.
This comment has been minimized.
Ah, I see what the error was now. I'll be more careful from hereon out, but I'd also be happy to just leave the |
This comment has been minimized.
This comment has been minimized.
I guess that's an option. But I was suggesting that we generally shouldn't touch translators unless we have either reports of failures or failing tests, because we're more likely to introduce new bugs than to fix things that caused actual problems before. Before starting work on fixes on actual fixes, whoever is doing the fixing would do a linting step using our new tools. You're right that there's a difference between manual fixes and automatic fixes, but not everything can be fixed automatically, and so if it's going to take manual work anyway, and we can't guarantee that current tests will catch all bugs, I'd rather the linting be done by someone who's actually working on the code and updating it for the current version of the site at the time. The linting would just be a separate initial commit in the PR that fixes the reported problem. The downside is that someone trying to do a quick translator fix would need to at least run I'd defer to @adam3smith and @zuphilip on this, though. |
This comment has been minimized.
This comment has been minimized.
I'm open to look at failing tests. |
retorquere commentedFeb 25, 2019
also shows that there must be an error in the translator -- jabrefMap is called but not defined.