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

Ci check deleted.txt when translators are deleted #1238

Merged
merged 11 commits into from Jan 29, 2017
+112 −25

Conversation

Reviewers

@adam3smith adam3smith

@dstillman dstillman

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@zuphilip @adam3smith @dstillman @kba
@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

@kba
ci: check translators for reused IDs
bfff6e8
@kba
ci: check for deleted.txt consistency
25ef352
@kba @zuphilip
ci: check translators for reused IDs
9b91cf2
@kba @zuphilip
ci: check for deleted.txt consistency
341a5ed
@zuphilip
Improve checkDeletedTxt.sh a little
d724582
@zuphilip
Add multistr option to jshintrc
a1f8b68
@kba
Merge branch 'ci-deletedtxt', only check translator JS for deletion
113883e
@kba
rev-parse the HEAD revision without abbrev-ref
d788960
@zuphilip
.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.
62ba1ef
@zuphilip
CI: Update Readme, use stable path for jshintrc
Loading status checks…
df69925
@adam3smith

adam3smith reviewed Jan 27, 2017
View changes

.ci/README.md Outdated
```
export SKIP_WARN=true
./checkTranslator.sh ../Amazon.com
./checkTranslator.sh --skip-warn ../Amazon.com

This comment has been minimized.

Sign in to view
Copy link
@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.

Sign in to view
Copy link
@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.

Sign in to view
Copy link
@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.

Sign in to view
Copy link
@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.

Sign in to view
Copy link
@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.

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

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

1 check passed

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

This comment has been minimized.

Sign in to view
Copy link
Collaborator

adam3smith commented Jan 29, 2017

excellent! Thanks
@zuphilip

This comment has been minimized.

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

Closed

Travis: Add tests for deleting files #1226

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

@zuphilip
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
af9e6fe

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

@zuphilip
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
d50f22f
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.