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 upCi check deleted.txt when translators are deleted #1238
Conversation
kba
and others
added some commits
Jan 19, 2017
adam3smith
reviewed
Jan 27, 2017
``` | ||
export SKIP_WARN=true | ||
./checkTranslator.sh ../Amazon.com | ||
./checkTranslator.sh --skip-warn ../Amazon.com |
This comment has been minimized.
This comment has been minimized.
adam3smith
Jan 27, 2017
Collaborator
question: is there are reason the script needs to use the awkward ../Amazon.com syntax or could we add the ../ into the script so that this would be /./checkTranslator.sh --skip-warn Amazon.com
This comment has been minimized.
This comment has been minimized.
dstillman
Jan 27, 2017
Member
I think that's a mistake anyway — it assume it should be ../Amazon.com.js
.
We could have it treat the given name as a label (and have that work regardless of the current working directory — so .ci/checkTranslator.sh Amazon.com
would work too), but it should also take a filename so that you can get filename completion.
This comment has been minimized.
This comment has been minimized.
zuphilip
Jan 27, 2017
Author
Collaborator
Yes, the missing .js
is a mistake, however the Amazon translator was renamed to Amazon.js
without the .com
, because it works on several (localized) Amazon websites. I always use the filename completion (especially if the filename contains spaces) and therefore didn't saw my error earlier.
This works after this PR: .ci/checkTranslator.sh Amazon.js
.
We can do some of the other "simplifications" but I am not sure how much we want of these. Let me know what you think we should (essentially) support:
- In the
translators
directory
a).ci/checkTranslator.sh Amazon.js
✔️
b).ci/checkTranslator.sh Amazon
❔ - In the
.ci
directory
a)./checkTranslator.sh ../Amazon.js
✔️
b)./checkTranslator.sh Amazon.js
❔
c)./checkTranslator.sh Amazon
❔ ❔
This comment has been minimized.
This comment has been minimized.
dstillman
Jan 27, 2017
Member
We should support path and label (filename without .js
). We don't need to support filename (but if you're in the root, the filename is the same as the path).
So:
- In the
translators
directory
a).ci/checkTranslator.sh Amazon.js
✔
b).ci/checkTranslator.sh Amazon
✔ - In the
.ci
directory
a)./checkTranslator.sh ../Amazon.js
✔️
b)./checkTranslator.sh Amazon.js
✘
c)./checkTranslator.sh Amazon
✔
This comment has been minimized.
This comment has been minimized.
dstillman
Jan 27, 2017
Member
So, basically, we just need to test whether path exists as-is, and if it doesn't try $ROOTDIR/$LABEL.js
(or whatever the variables are).
This comment has been minimized.
This comment has been minimized.
I have one question above, otherwise this looks great to me. |
This comment has been minimized.
This comment has been minimized.
Okay, I added the option to reference the translators just by label and updated the README file. Please have a look at the new version. |
adam3smith
merged commit bb460b0
into
zotero:master
Jan 29, 2017
1 check passed
This comment has been minimized.
This comment has been minimized.
excellent! Thanks |
This comment has been minimized.
This comment has been minimized.
Thanks to @kba for making my first messy attempts into scripts which work excellently and have well organized code! |
zuphilip commentedJan 27, 2017
Add another script for checking the
deleted.txt
, which has to be updated when deleting translators. This will work forThough, we should just make sure that all deletions of translators are done as PR and not direct pushes, but this might be anyway a good idea.
There are also some small improvements for the
checkTranslator.sh
.This is joint work with @kba (the shell script ninja)🙇 .