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

tools: fix ICU shrinker and docs #23266

Merged
merged 0 commits into from
Oct 4, 2018
Merged

Conversation

srl295
Copy link
Member

@srl295 srl295 commented Oct 4, 2018

  • tools: path to ICU datafile moved
  • docs: configure is now configure.py

Fixes: #23245

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added i18n-api Issues and PRs related to the i18n implementation. tools Issues and PRs related to the tools directory. labels Oct 4, 2018
@srl295 srl295 self-assigned this Oct 4, 2018
@srl295 srl295 requested review from refack and devsnek October 4, 2018 18:40
@srl295
Copy link
Member Author

srl295 commented Oct 4, 2018

This is just a minimal change, doesn't need to wait for ICU 63.1. @refack I'd like to try your comment in
#23245 (comment) alongside an actual ICU update.

@refack
Copy link
Contributor

refack commented Oct 4, 2018

@srl295 do you know what caused this change in path (just curious)

@refack
Copy link
Contributor

refack commented Oct 4, 2018

This is just a minimal change, doesn't need to wait for ICU 63.1. @refack I'd like to try your comment in
#23245 (comment) alongside an actual ICU update.

I would be happy to help.

@srl295
Copy link
Member Author

srl295 commented Oct 4, 2018

@refack not sure. I looked briefly into it but didn't find anything obvious.

@srl295
Copy link
Member Author

srl295 commented Oct 4, 2018

Landed in a4ffa0c

@srl295 srl295 closed this Oct 4, 2018
@srl295 srl295 merged commit a4ffa0c into nodejs:master Oct 4, 2018
@Trott
Copy link
Member

Trott commented Oct 5, 2018

This landed after being open for 3 hours with just one approval (needs two) and without a full CI run (the lite-CI-only is probably OK if the code is not exercised by our test suite).

For small changes that don't need to wait the full time, we have a simple and light-weight fast-tracking procedure.

There are a few people advocating for getting rid of the wait period and this PR will no doubt be seen by them as a case-in-point of why we should do that.

If we are going to have those kinds of rules, it would be great if they were enforced by tooling (e.g., a commit queue). Occasional contributors especially are likely to not be aware of the rules or that there have been changes.

@Trott
Copy link
Member

Trott commented Oct 5, 2018

Also: After-the-fact LGTM. I also endorse fast-tracking this. Feel free to 👍 here to likewise endorse fast-tracking although it's obviously just symbolic at this point.

@Trott Trott added the fast-track PRs that do not need to wait for 48 hours to land. label Oct 5, 2018
targos pushed a commit that referenced this pull request Oct 5, 2018
- tools: path to ICU datafile moved
- docs: configure is now configure.py

Fixes: #23245

PR-URL: #23266
Reviewed-By: Refael Ackermann <[email protected]>
@srl295
Copy link
Member Author

srl295 commented Oct 5, 2018

This landed after being open for 3 hours with just one approval (needs two)

Sorry, I thought I had already had 2. (Related: nodejs/node-core-utils#284 )

getting rid of the wait period

OK, I need to reread the procedures and the thread, I thought it said I had only a few hours left in which to land the change. I didn't realize it was a minimum! edit Opened a PR to update git-node's message nodejs/node-core-utils#287

Also, I was trying to avoid merge conflicts with documentation in
#23269 - that was my reason for landing this separate from the main ICU bump PR. Again, was not trying to circumvent procedure here though.

@Trott
Copy link
Member

Trott commented Oct 5, 2018

@srl295 No worries. This is all a pretty good demonstration of all sorts of problems with our rules (too many of them, too confusing, too many people that have to know about them, not enough enforcement via tools, too many variations, etc.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fast-track PRs that do not need to wait for 48 hours to land. i18n-api Issues and PRs related to the i18n implementation. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants