-
Notifications
You must be signed in to change notification settings - Fork 102
Fix failure setting optimize option (#480) #481
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
Conversation
Codecov ReportBase: 87.34% // Head: 87.37% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #481 +/- ##
==========================================
+ Coverage 87.34% 87.37% +0.02%
==========================================
Files 39 39
Lines 2316 2320 +4
==========================================
+ Hits 2023 2027 +4
Misses 293 293
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
|
@mkitti do you think you could merge this? (You have merge access, right?) |
|
We would need to get these tests passing |
|
@MilesCranmer Clone the branch, get the tests to pass, and submit a new PR pinging me. Then I will merge it. |
|
logs have expired. Can sb rerun the tests? |
|
Considering rebasing |
e097806 to
355cf39
Compare
|
@mkitti done |
|
Hmm, that did not seem to fix testing. |
|
It seems unrelated to this PR - the issue seems to be because the CI is trying to download a pip package from an HTTP URL instead of HTTPS, and urllib no longer allows non-SSL requests: I think that's why the Python 2.7 version works - since the old urllib hasn't deprecated non-SSL. |
355cf39 to
c8f5f64
Compare
|
@MilesCranmer rebased. thanks |
c8f5f64 to
8fcb86e
Compare
|
@mkitti any idea how to change the actions for “Required statuses must pass before merging”? It seems like they are for a different CI naming scheme maybe. |
MilesCranmer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
Let me take a look at the branch protection rules. |
| (dict(compiled_modules="no"), ["--compiled-modules", "no"]), | ||
| (dict(depwarn="error"), ["--depwarn", "error"]), | ||
| (dict(sysimage="PATH"), ["--sysimage", "PATH"]), | ||
| (dict(bindir="PATH"), ["--home", "PATH"]), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happened to bindir?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than this merge when ready.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is bindir an option in Julia anymore? I don't see it in the --help menu.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure, I can't even use it locally:
julia --bindir=$PATH
# ERROR: unknown option `--bindirThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's replaced by --home?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah, there it is:
def cli_argument_name(self):
name = {"bindir": "home"}.get(self.name, self.name)So this should not be removed @dpinol
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I readded it.
|
Status checks should be functional now |
|
Could this one be merged? |
|
Awesome! 🚀 |
Fixes #480