Skip to content

/translators

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

Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@zuphilip @adam3smith
@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).
@zuphilip

zuphilip reviewed Jan 1, 2017
View changes

National Gallery of Art - USA.js Outdated
@@ -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.

Sign in to view
Copy link
@zuphilip

zuphilip Jan 1, 2017 •
edited

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.

Sign in to view
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

@zuphilip
Always start target with ^, when appropriate
74e64cf
@zuphilip
Start target always with ^https?, when approriate
e9f5a97
@zuphilip
Don't escape slash in target regexp
0123b68
@zuphilip
Escape periods in target when needed 
Found by

$ grep '"target": .*[^\]\.\w' *.js
5521075
@zuphilip
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.
e7957a7

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

@zuphilip

This comment has been minimized.

Sign in to view
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 zuphilip referenced this pull request Jan 8, 2017

Closed

Target regexp and priority cleanup #789

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

@zuphilip
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.
8a2204e

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

@zuphilip
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.
3f189b8
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.