Skip to content

/ servo

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

Rename check_no_unwrap.sh to check_no_panic.sh #10942

Merged
merged 1 commit into from Oct 29, 2016
Merged

Rename check_no_unwrap.sh to check_no_panic.sh #10942

merged 1 commit into from Oct 29, 2016
+3 −4

Conversation

@6br
Copy link
Contributor

6br commented Apr 30, 2016 •
edited by larsbergstrom

Hello, I fixed #10930.

This change is Reviewable
@highfive
Copy link

highfive commented Apr 30, 2016

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @metajack (or someone else) soon.
@highfive
Copy link

highfive commented Apr 30, 2016

Heads up! This PR modifies the following files:

  • @aneeshusa: etc/ci/check_no_panic.sh, etc/ci/check_no_unwrap.sh, etc/ci/buildbot_steps.yml
etc/ci/check_no_panic.sh Outdated
FILES=("components/compositing/compositor.rs"
"components/compositing/pipeline.rs"
"components/compositing/constellation.rs")

! grep -n "unwrap(\|panic!(" "${FILES[@]}"
! grep -n "panic(\|panic!(" "${FILES[@]}"

This comment has been minimized.

Sign in to view
@KiChjang

KiChjang Apr 30, 2016

Member

This looks very wrong to me.
@KiChjang
Copy link
Member

KiChjang commented Apr 30, 2016

Could you also claim the issue saying that you're working on it next time? This is to prevent having two or more people working on the same issue.
@6br 6br force-pushed the 6br:rename_check_no_panic branch from be78f0c to c44fd69 Apr 30, 2016
@6br
Copy link
Contributor Author

6br commented Apr 30, 2016

I'm sorry for not having written in advance, and I'll take care from now on.
@6br 6br force-pushed the 6br:rename_check_no_panic branch from c44fd69 to f9e2217 Apr 30, 2016
@KiChjang
Copy link
Member

KiChjang commented Apr 30, 2016

r? @aneeshusa
etc/ci/check_no_panic.sh Outdated
set -o errexit
set -o nounset
set -o pipefail

cd $(git rev-parse --show-toplevel) # cd into repo root so make sure paths works in any case

# files that should not contain "unwrap"
# files that should not contain "panic"

This comment has been minimized.

Sign in to view
@aneeshusa

aneeshusa Apr 30, 2016

Member

I actually think we can get rid of this comment, the first one makes this redundant.
etc/ci/check_no_panic.sh Outdated
@@ -1,13 +1,13 @@
#!/bin/bash
#
# Make sure listed files do not contain "unwrap"
# Make sure listed files do not contain "panic"

This comment has been minimized.

Sign in to view
@aneeshusa

aneeshusa Apr 30, 2016

Member

Let's say ...do not use unwrap() or panic!() instead.
@aneeshusa
Copy link
Member

aneeshusa commented Apr 30, 2016

@6br thanks for the PR! As I mentioned in the issue, this is blocked on servo/saltfs#316, so it may be a while before this is merged. (It's meant to be an example/test run for having CI steps in tree - this isn't a particularly urgent change.)

Also, good catch about updating .travis.yml, and good thinking about updating the comment!
@6br 6br force-pushed the 6br:rename_check_no_panic branch from f9e2217 to c113d1e May 1, 2016
@6br
Copy link
Contributor Author

6br commented May 1, 2016

Thanks for your review. I fixed the comments.
@bors-servo
Copy link
Contributor

bors-servo commented May 1, 2016

☔ The latest upstream changes (presumably #10948) made this pull request unmergeable. Please resolve the merge conflicts.
@bors-servo
Copy link
Contributor

bors-servo commented May 19, 2016

☔ The latest upstream changes (presumably #11249) made this pull request unmergeable. Please resolve the merge conflicts.
@6br 6br force-pushed the 6br:rename_check_no_panic branch from 49fbe1e to 870016c May 20, 2016
@bors-servo
Copy link
Contributor

bors-servo commented May 20, 2016

☔ The latest upstream changes (presumably #10910) made this pull request unmergeable. Please resolve the merge conflicts.
@jdm jdm removed the S-awaiting-review label Jun 8, 2016
@emilio
Copy link
Member

emilio commented Jul 4, 2016

Is this still active?

@aneeshusa, it seems to be awaiting for your review (though it now needs a rebase from @6br too).
@jdm
Copy link
Member

jdm commented Jul 4, 2016

Still blocked on servo/saltfs#316.
@emilio
Copy link
Member

emilio commented Jul 4, 2016

Huh, true, sorry for not noticing
@bors-servo
Copy link
Contributor

bors-servo commented Sep 23, 2016

☔ The latest upstream changes (presumably #13383) made this pull request unmergeable. Please resolve the merge conflicts.
@6br 6br force-pushed the 6br:rename_check_no_panic branch from dd60f38 to 5386c52 Sep 26, 2016
@bors-servo
Copy link
Contributor

bors-servo commented Oct 13, 2016

☔ The latest upstream changes (presumably #13722) made this pull request unmergeable. Please resolve the merge conflicts.
@6br 6br force-pushed the 6br:rename_check_no_panic branch from 5386c52 to f9adb99 Oct 19, 2016
@aneeshusa
Copy link
Member

aneeshusa commented Oct 28, 2016

servo/saltfs#316 has been fixed and it looks like all the kinks have been worked out, so this is no longer blocked. Thanks for your patience while that was fixed!

This needs one more change before merging: we need to update the steps for linux-dev in etc/ci/buildbot_steps.yml.
@aneeshusa aneeshusa assigned aneeshusa and unassigned metajack Oct 28, 2016
@bors-servo
Copy link
Contributor

bors-servo commented Oct 29, 2016

☔ The latest upstream changes (presumably #13968) made this pull request unmergeable. Please resolve the merge conflicts.
@6br
Rename check_no_unwrap.sh to check_no_panic.sh
Loading status checks…
81f2fae
@6br 6br force-pushed the 6br:rename_check_no_panic branch from f9adb99 to 81f2fae Oct 29, 2016
@6br
Copy link
Contributor Author

6br commented Oct 29, 2016

Sure, I fixed etc/ci/buildbot_steps.yml.
@aneeshusa
Copy link
Member

aneeshusa commented Oct 29, 2016

@bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented Oct 29, 2016

📌 Commit 81f2fae has been approved by aneeshusa
@bors-servo
Copy link
Contributor

bors-servo commented Oct 29, 2016

⌛ Testing commit 81f2fae with merge 3ae0a33...
bors-servo added a commit that referenced this pull request Oct 29, 2016
@bors-servo
Auto merge of #10942 - 6br:rename_check_no_panic, r=aneeshusa
Loading status checks…
3ae0a33 
Rename check_no_unwrap.sh to check_no_panic.sh

Hello, I fixed #10930.

<!-- 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/10942)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Oct 29, 2016

☀️ Test successful - arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-dev
@bors-servo bors-servo merged commit 81f2fae into servo:master Oct 29, 2016
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@aneeshusa aneeshusa

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

Rename check_no_unwrap.sh to check_no_panic.sh
9 participants
@6br @highfive @KiChjang @aneeshusa @bors-servo @emilio @jdm @metajack @wafflespeanut
You can’t perform that action at this time.