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

fixed a few typos #209

Closed
wants to merge 1 commit into from
Closed

fixed a few typos #209

wants to merge 1 commit into from

Conversation

Ashafix
Copy link
Contributor

@Ashafix Ashafix commented Apr 20, 2019

Encountered the function 'is_anacoda` while looking at issue #205, renamed it and fixed a few more typos.

@peternowee
Copy link
Member

Reviewed. Looks good to me. Found no other occurrences of the renamed function, and the other two corrections are in comments. All tests pass (when run together with #211).

@peternowee
Copy link
Member

@Ashafix I rebased and merged your commit as a763960. It will be part of the next release, probably 1.4.2. Thanks!

@peternowee peternowee closed this Aug 27, 2020
@peternowee
Copy link
Member

@Ashafix I notice that GitHub does not recognize your PR as having been merged. I rebased and merged locally, then pushed to GitHub. I was not sure what it would do. Next time I better try to give you some time to rebase it yourself if you want, although that is difficult if there are multiple PRs waiting to be merged. Or maybe next time I should use the GitHub interface instead of work locally, although that brings limitations with regard to signing. Related discussions in isaacs/github#2, isaacs/github#88 and isaacs/github#361.

Anyway, I wonder what would happen if you would force-push the new commit a763960 over your branch Ashafix:master: If GitHub would then suddenly consider this PR to have been merged. I will reopen this PR for a few days in case you also want to try that out.

@peternowee peternowee reopened this Aug 27, 2020
@peternowee
Copy link
Member

@Ashafix On second thought, overwriting your branch with the new commit probably will not work either, because then your branch will just be equal to pydot:master, which will also close it automatically. I think we just have to forget about the final status of this PR according to GitHub. Sorry, hope you do not mind too much and I will see what I can do better next time. At least the commit log correctly shows your work was merged and that you were the Author; I hope you agree that is the most important. Again, thank you for your contribution and I will contact you about the PEP8 cleanup in the discussion below Ashafix@38da0de later.

@Ashafix
Copy link
Contributor Author

Ashafix commented Aug 30, 2020

@peternowee : thanks for taking care of it!

I saw that two unittests are failing.
Should I be looking into that or is something unrelated to the PR?

@peternowee
Copy link
Member

@Ashafix You mean the failing AppVeyor ones, right? See #213 for that. Help is welcome, as I don't have Windows myself, so I cannot even start to try to reproduce it locally. With some effort, it is possible to access the AppVeyor VM's, but only for a limited time after each run. It seems to be a very AppVeyor-specific problem, since I did not see any reports of this problem yet, so it may be difficult/impossible to reproduce on a local Windows-installation. Maybe AppVeyor support needs to be contacted. Anyway, for now I just ignore the AppVeyor failures on those specific tests.

@peternowee peternowee closed this Aug 31, 2020
@peternowee
Copy link
Member

And just to answer your question more clearly: No, these two failing tests at AppVeyor are not related to this PR, but of course any help is welcome on solving them. Again, see #213.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants