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

Ci check deleted.txt when translators are deleted #1238

Merged
merged 11 commits into from Jan 29, 2017

Conversation

Projects
None yet
4 participants
@zuphilip
Copy link
Collaborator

zuphilip commented Jan 27, 2017

Add another script for checking the deleted.txt, which has to be updated when deleting translators. This will work for

  • pull requests against the master branch
  • locally when working on feature branch

Though, 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) 🙇 .

kba and others added some commits Jan 19, 2017

.ci/checkDeletedTxt.sh: Warn/exit if master not available
Actually, this script will either be used in a local feature branch
and then we can assume that master branch is also available, or
in the Travis check of the official repo, where in each PR the
master branch is available.

In all other cases we skip now the test and show just a warning
that we are doing that.
```
export SKIP_WARN=true
./checkTranslator.sh ../Amazon.com
./checkTranslator.sh --skip-warn ../Amazon.com

This comment has been minimized.

@adam3smith

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.

@dstillman

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.

@zuphilip

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:

  1. In the translators directory
    a) .ci/checkTranslator.sh Amazon.js ✔️
    b) .ci/checkTranslator.sh Amazon
  2. In the .ci directory
    a) ./checkTranslator.sh ../Amazon.js ✔️
    b) ./checkTranslator.sh Amazon.js
    c) ./checkTranslator.sh Amazon

This comment has been minimized.

@dstillman

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:

  1. In the translators directory
    a) .ci/checkTranslator.sh Amazon.js
    b) .ci/checkTranslator.sh Amazon
  2. In the .ci directory
    a) ./checkTranslator.sh ../Amazon.js ✔️
    b) ./checkTranslator.sh Amazon.js
    c) ./checkTranslator.sh Amazon

This comment has been minimized.

@dstillman

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).

@adam3smith

This comment has been minimized.

Copy link
Collaborator

adam3smith commented Jan 27, 2017

I have one question above, otherwise this looks great to me.

@zuphilip

This comment has been minimized.

Copy link
Collaborator Author

zuphilip commented Jan 28, 2017

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 adam3smith merged commit bb460b0 into zotero:master Jan 29, 2017

1 check passed

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

This comment has been minimized.

Copy link
Collaborator

adam3smith commented Jan 29, 2017

excellent! Thanks

@zuphilip

This comment has been minimized.

Copy link
Collaborator Author

zuphilip commented Jan 29, 2017

Thanks to @kba for making my first messy attempts into scripts which work excellently and have well organized code!

@zuphilip zuphilip referenced this pull request Jan 29, 2017

Closed

Ci deletedtxt #2

@zuphilip zuphilip deleted the infolis:ci-deletedtxt branch Feb 19, 2017

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

Ci check deleted.txt when translators are deleted (zotero#1238)
* ci: check translators for reused IDs
* ci: check for deleted.txt consistency
* Add multistr option to jshintrc
* .ci/checkDeletedTxt.sh: Warn/exit if master not available
Actually, this script will either be used in a local feature branch
and then we can assume that master branch is also available, or
in the Travis check of the official repo, where in each PR the
master branch is available.

In all other cases we skip now the test and show just a warning
that we are doing that.
* Add option to call checkTranslator.sh with label only

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

Ci check deleted.txt when translators are deleted (zotero#1238)
* ci: check translators for reused IDs
* ci: check for deleted.txt consistency
* Add multistr option to jshintrc
* .ci/checkDeletedTxt.sh: Warn/exit if master not available
Actually, this script will either be used in a local feature branch
and then we can assume that master branch is also available, or
in the Travis check of the official repo, where in each PR the
master branch is available.

In all other cases we skip now the test and show just a warning
that we are doing that.
* Add option to call checkTranslator.sh with label only
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.