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

Target tweaks #1211

Merged
merged 5 commits into from Jan 8, 2017

Conversation

Projects
None yet
2 participants
@zuphilip
Copy link
Collaborator

zuphilip commented Jan 1, 2017

Two years ago @aurimasv started a PR #789 to cleanup the target regexp a little and make it more consistent. This old PR has many conflicts now because since then many of the translators where touched. Therefore, I just started over to achieve partially the same goals (I concentrate on the things where grep can at least find the cases easily):

  • The target starts now with ´^https?` always, when appropriate.
grep '"target": "[^\^]' *.js
grep '"target": "^[^h]' *.js
grep '"target": "^h[^t]' *.js
grep '"target": "^ht[^t]' *.js
grep '"target": "^htt[^p]' *.js
grep '"target": "^http[^s]' *.js
grep '"target": "^https[^?]' *.js
grep '"target": "^https?[^:]' *.js
grep '"target": "^https?:[^/]' *.js
grep '"target": "^https?:/[^/]' *.js
  • There is no need to escape slashes / in regexp and therefore I simplified all \\/.
grep '"target": .*\\/' *.js
  • Periods in the target were not always escaped when clearly meant to be there literally.
grep '"target": .*[^\]\.\w' *.js
  • Grouping in target don't need to distinguish between captering and non-captering groups. Therefore, I replaced all (?:with simply (.
grep '"target": .*[(][?]:' *.js

I hope this is easy to review and we can merge this PR fast (before there will any conflicts occurring).

@@ -1,8 +1,8 @@
{
"translatorID": "ed28758b-9c39-4e1c-af89-ce1c9202b70f",
"label": "National Gallery of Art - U.S.A.",
"label": "National Gallery of Art - USA",

This comment has been minimized.

@zuphilip

zuphilip Jan 1, 2017

Author Collaborator

Changed the label here as well to be consistent with the file name (otherwise Scaffold will always trying to rename it).

@adam3smith

This comment has been minimized.

Copy link
Collaborator

adam3smith commented Jan 7, 2017

if you wait until we finish with the O translators and then rebase this, I'll merge immediately.

zuphilip added some commits Jan 1, 2017

Escape periods in target when needed
Found by

$ grep '"target": .*[^\]\.\w' *.js
Simplify groups in regexp of target
Remove all instances of non-capturing parentheses and replace with capturing parentheses. This doesn't matter in target regexp and makes it a bit cleaner.

@zuphilip zuphilip force-pushed the zuphilip:target-tweaks branch from d324600 to e7957a7 Jan 8, 2017

@zuphilip

This comment has been minimized.

Copy link
Collaborator Author

zuphilip commented Jan 8, 2017

I already rebased that one and there seems to be no new conflict after merging #1214.

@adam3smith adam3smith merged commit ed21c7b into zotero:master Jan 8, 2017

@zuphilip zuphilip deleted the zuphilip:target-tweaks branch Jan 8, 2017

zuphilip added a commit to zuphilip/translators that referenced this pull request Mar 28, 2018

Target tweaks (zotero#1211)
Closes zotero#789
The priority issue is separate, but we may also have done that since.
* Always start target with ^, when appropriate
* Start target always with ^https?, when approriate
* Don't escape slash in target regexp
* Escape periods in target when needed
Found by
$ grep '"target": .*[^\]\.\w' *.js
* Simplify groups in regexp of target
Remove all instances of non-capturing parentheses and replace with capturing parentheses. This doesn't matter in target regexp and makes it a bit cleaner.

zuphilip added a commit to zuphilip/translators that referenced this pull request Mar 28, 2018

Target tweaks (zotero#1211)
Closes zotero#789
The priority issue is separate, but we may also have done that since.
* Always start target with ^, when appropriate
* Start target always with ^https?, when approriate
* Don't escape slash in target regexp
* Escape periods in target when needed
Found by
$ grep '"target": .*[^\]\.\w' *.js
* Simplify groups in regexp of target
Remove all instances of non-capturing parentheses and replace with capturing parentheses. This doesn't matter in target regexp and makes it a bit cleaner.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.