8000 MAINT: Fix some typos in a code string and comments by dongjoon-hyun · Pull Request #7134 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

MAINT: Fix some typos in a code string and comments #7134

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 28, 2016
Merged

MAINT: Fix some typos in a code string and comments #7134

merged 1 commit into from
Jan 28, 2016

Conversation

dongjoon-hyun
Copy link
Contributor

No description provided.

@njsmith
Copy link
Member
njsmith commented Jan 28, 2016

Wow, thank you!

@homu: r+

@homu
Copy link
Contributor
homu commented Jan 28, 2016

📌 Commit 73a2fd1 has been approved by njsmith

homu added a commit that referenced this pull request Jan 28, 2016
@homu
Auto merge of #7134 - dongjoon-hyun:fix_typos_in_code_comments, r=njs…
e280539 
…mith

MAINT: Fix some typos in a code string and comments
@homu
Copy link
Contributor
homu commented Jan 28, 2016

⌛ Testing commit 73a2fd1 with merge e280539...

@homu
Copy link
Contributor
homu commented Jan 28, 2016

💔 Test failed - status

@njsmith
Copy link
Member
njsmith commented Jan 28, 2016

Huh, those are some weird failures that travis is throwing up :-/ I don't see how they can possibly be related to this PR, but does anyone know what's going on? that's 4 errors over 2 builds, 3 different error messages :-/

@dongjoon-hyun
Copy link
Contributor Author

In homu tests, I found the following error. Is that a reason?

sudo: debootstrap: command not found

@dongjoon-hyun
Copy link
Contributor Author

In travis-ci-/pr, I found another error.

./tools/travis-test.sh: line 61: python3-dbg: command not found

@dongjoon-hyun
Copy link
Contributor Author

I think I found the reason. Travis CI fails to install the required software due to network traffics (maybe the aftereffect of today's GitHub errors).

3.5 is not installed; attempting download
bzip2: Compressed file ends unexpectedly;
    perhaps it is corrupted?  *Possible* reason follows.
bzip2: Inappropriate ioctl for device
    Input file = (stdin), output file = (stdout)
It is possible that the compressed file(s) have become corrupted.
You can use the -tvv option to test integrity of such files.
You can use the `bzip2recover' program to attempt to recover
data from undamaged sections of corrupted files.
tar: Unexpected EOF in archive
tar: Unexpected EOF in archive
tar: Error is not recoverable: exiting now

rgommers
rgommers reviewed Jan 28, 2016
View reviewed changes
numpy/f2py/rules.py
@@ -15,14 +15,14 @@
get_b_from_python
if (successful) {

callfortran
if (succesful) {
call_fortran
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one shouldn't be changed I think - callfortran is used a lot in the rest of this file.

@rgommers
Copy link
Member

nice cleanup, thanks @dongjoon-hyun

@charris
Copy link
Member
charris commented Jan 28, 2016

Github went down hard for about five hours at ~5:20 PM MST yesterday and it took a while to recover.

@homu
Copy link
Contributor
homu commented Jan 28, 2016

☀️ Test successful - status

@homu homu merged commit 73a2fd1 into numpy:master Jan 28, 2016
This was referenced Jan 28, 2016
Closed
Merged
@dongjoon-hyun
Copy link
Contributor Author

Oh, thank you for merging!

@rgommers
Copy link
Member

@njsmith I don't like that auto-merge trick - having a bot as commit author is just wrong. plus practical reasons, like it's confusing and messes with commits counts etc.

@njsmith
Copy link
Member
njsmith commented Jan 28, 2016

Hmm, not sure what happened here this time -- I feel like it should have given up once the first build failed.

(Did someone hit the "rerun this build" button on Travis or something?)

The useful bits about the bot are that (1) it gives a way to say "merge if tests pass" sitting around and waiting for them, which is why I used it this time, and (2) by testing before merging it guarantees that master always passes tests (because -- unlike normal Travis runs -- it actually tests the merge that's being committed, instead of some merge with whatever matter looked at when the PR was first submitted, maybe weeks ago). There are a number of projects that use it exclusively for these reasons (esp. in the rust ecosystem).

The annoying things are, it's too chatty, and it doesn't support appveyor, so meh. I guess I could add it not setting the "committed by" field properly to my mental list of grievances? I'm not sure what to do with it being "just wrong" and "confusing" -- it definitely solves some real technical problems for some cases -- and I feel like I care more about prompt and rigorous testing than about commit counting tools, but yeah, it's definitely not an obvious 100% win :-/. Maybe with some fixes it could be...

@rgommers
Copy link
Member

I'm not sure what to do with it being "just wrong" and "confusing" -- it definitely solves some real technical problems for some cases --

Well, in this case it merged an open PR after I had made a comment on it. That confused me (I only noticed because of the "thanks for merging" comment), and is definitely not the right thing to do.

And @homu is clearly confusing the people that keep on replying to it:)

@njsmith
Copy link
Member
njsmith commented Jan 29, 2016

Yeah, like I said I have no idea why it did that :-/ the logic is supposed
to be, if someone approves the PR then it runs the tests, and either merges
or not depending on the outcome. Somehow here it ran the tests, they
failed, it announced that it wasn't merging, and then it ran them again (??
or something caused them to be rerun), and this time they passed so it
merged?
.
@barosl: any idea what might be going on here?
On Jan 29, 2016 12:50 AM, "Ralf Gommers" notifications@github.com wrote:

I'm not sure what to do with it being "just wrong" and "confusing" -- it
definitely solves some real technical problems for some cases --

Well, in this case it merged an open PR after I had made a comment on
it. That confused me (I only noticed because of the "thanks for merging"
comment), and is definitely not the right thing to do.

And @homu is clearly confusing the people that keep on replying to it:)

—
Reply to this email directly or view it on GitHub
#7134 (comment).

@charris
Copy link
Member
charris commented Jan 29, 2016

On Fri, Jan 29, 2016 at 2:49 AM, Nathaniel J. Smith <
notifications@github.com> wrote:

Yeah, like I said I have no idea why it did that :-/ the logic is supposed
to be, if someone approves the PR then it runs the tests, and either merges
or not depending on the outcome. Somehow here it ran the tests, they
failed, it announced that it wasn't merging, and then it ran them again (??
or something caused them to be rerun), and this time they passed so it
merged?

I reran the tests. When github came back on line tests failed because not
all of the network was yet up, so I reran them.

Chuck

@dongjoon-hyun
Copy link
Contributor Author

Hi, all, Sorry for interrupting your discussion.
According to the context, I think it's accidentally merged during under reviews and the following should be fixed. Am I right? :)

This one shouldn't be changed I think - callfortran is used a lot in the rest of this file.
Add a line note

How can I recover the callfortran stuff? Should I make another PR for that line? Please give me some advice.

@rgommers
Copy link
Member

@dongjoon-hyun no worries. A new PR for that would be good.

@dongjoon-hyun
Copy link
Contributor Author
dongjoon-hyun commented 8000 Jan 29, 2016

Okay, thank you, @rgommers . I'll make a new MAINT PR for that.

-      call_fortran
+      callfortran

dongjoon-hyun added a commit to dongjoon-hyun/numpy that referenced this pull request Jan 29, 2016
@dongjoon-hyun
MAINT: Change call_fortran into callfortran in comments.
99977a5 
It was committed mistakenly in numpy#7134. `callfortran` is used a lot really.
@dongjoon-hyun dongjoon-hyun mentioned this pull request Jan 29, 2016
Merged
@njsmith
Copy link
Member
njsmith commented Jan 30, 2016

@charris: Ahhh, okay, at least now we know what happened :-) Pretty weird edge case here...

jaimefrio pushed a commit to jaimefrio/numpy that referenced this pull request Mar 22, 2016
@dongjoon-hyun @jaimefrio
MAINT: Change call_fortran into callfortran in comments.
9c44346 
It was committed mistakenly in numpy#7134. `callfortran` is used a lot really.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
03 - Maintenance
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dongjoon-hyun @njsmith @homu @rgommers @charris
0