Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upAdded test that files exist to etc/ci/check_no_unwrap.sh. #11251
Conversation
highfive
commented
May 18, 2016
|
Heads up! This PR modifies the following files:
|
|
Should land after #11249. |
| ls -1 "${FILES[@]}" | ||
|
|
||
| # make sure the files do not contain "unwrap" or "panic!" | ||
| ! grep -Hn "unwrap(\|panic!(" "${FILES[@]}" |
This comment has been minimized.
This comment has been minimized.
aneeshusa
May 18, 2016
Member
-H is the default when there is more than one file to search, so it's redundant here.
This comment has been minimized.
This comment has been minimized.
asajeffrey
May 18, 2016
Author
Member
True, I guess I prefer being explicit. I can remove it if you think it's confusing.
This comment has been minimized.
This comment has been minimized.
aneeshusa
May 18, 2016
Member
In that case, I think using long options would be better here. I've never seen -H used and had to look it up, and might as well spell out -n as well.
This comment has been minimized.
This comment has been minimized.
|
Yay, the build is failing like it should!: |
highfive
commented
May 18, 2016
|
New code was committed to pull request. |
| @@ -17,4 +17,4 @@ FILES=("components/compositing/compositor.rs" | |||
| ls -1 "${FILES[@]}" | |||
|
|
|||
| # make sure the files do not contain "unwrap" or "panic!" | |||
| ! grep -Hn "unwrap(\|panic!(" "${FILES[@]}" | |||
| ! grep --line-number --with-filename "unwrap(\|panic(" "${FILES[@]}" | |||
This comment has been minimized.
This comment has been minimized.
|
Argh, for some reason GitHub is showing my last comment as outdated. I'll repeat it here:
|
highfive
commented
May 18, 2016
|
New code was committed to pull request. |
|
Oops, added the exclamation point back in. |
| ls -1 "${FILES[@]}" | ||
|
|
||
| # make sure the files do not contain "unwrap" or "panic!" | ||
| ! grep --line-number --with-filename "unwrap(\|panic!(" "${FILES[@]}" |
This comment has been minimized.
This comment has been minimized.
aneeshusa
May 19, 2016
Member
style nit: There's an extra space between --with-filename and "unwrap(\panic!(".
|
r=me after fixing the spacing there and squashing. |
|
Thanks! I'll r=you after I fix, squash, and #11249 has been r+d by some kind soul (hint hint). |
|
Looks like @KiChjang beat me to it. |
|
Yay! |
asajeffrey
force-pushed the
asajeffrey:check-no-unwrap-check-for-files
branch
from
4d1ae9f
to
a6990b2
highfive
commented
May 19, 2016
|
New code was committed to pull request. |
|
@bors-servo r=aneeshusa |
|
|
|
|
…aneeshusa Added test that files exist to etc/ci/check_no_unwrap.sh. Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy --faster` does not report any errors - [X] No github issue. Either: - [ ] There are tests for these changes OR - [X] These changes do not require tests because this PR updates test code. Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11251) <!-- Reviewable:end -->
|
|
asajeffrey commentedMay 18, 2016
•
edited by larsbergstrom
Thank you for contributing to Servo! Please replace each
[ ]by[X]when the step is complete, and replace__with appropriate data:./mach build -ddoes not report any errors./mach test-tidy --fasterdoes not report any errorsEither:
Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process.
This change is