Skip to content

Update bst.py#54

Closed
ghost wants to merge 1 commit into
masterfrom
unknown repository
Closed

Update bst.py#54
ghost wants to merge 1 commit into
masterfrom
unknown repository

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented Oct 14, 2018

I am new to data structures so correct me if I'm wrong! I think you have some unnecessary code in your _left_rotation and _right_rotation methods. After setting pivot.left = pivot.right, the parent is already guaranteed to be the pivot, so you don't need to update it with the if block. Right? Also, the last line of code in each method doesn't need to update the parent of the pivot because the parent does not change … I think. Unless it is for some special case maybe? I am still learning so take my suggestions with a grain of salt. Thanks!

I am new to data structures so correct me if I'm wrong! I think you have some unnecessary code in your _left_rotation and _right_rotation methods. After setting pivot.left = pivot.right, the parent is already guaranteed to be the pivot, so you don't need to update it with the if block. Right? Also, the last line of code in each method doesn't need to update the parent of the pivot because the parent does not change … I think. Unless it is for some special case maybe? I am still learning so take my suggestions with a grain of salt. Thanks!
@jonathanstallings jonathanstallings self-assigned this Oct 15, 2018
@jonathanstallings
Copy link
Copy Markdown
Owner

Oh my. I haven't touched this code in a while. 😄

Good eye! You are correct that the logic you have identified is unnecessary, and your changes check out. Nice work. 👍

Do you mind updating your commit message with a more concise note indicating the reason for the change? I will be happy to merge that in.

Thank you for your contribution and good luck with your studies!

@ghost
Copy link
Copy Markdown
Author

ghost commented Oct 15, 2018

I do apologize. I am very new to git, GitHub, and programming and obviously have no idea what I am doing so I just created a new pull request!

@jonathanstallings
Copy link
Copy Markdown
Owner

jonathanstallings commented Oct 15, 2018

Oh, there is absolutely no need to apologize! You've come across this repo, forked it to your own account, read through the code and found an opportunity to refactor, and submitted a pull request to improve the codebase for others. That's awesome in my book!

As for the commit message format, well, that's a bit on me as well since I have not provided a Contributing guide for this repository. This was a small project I put together during my own studies a few years ago, and I had not anticipated outside contributors joining in. Thanks again for that. Also, full disclosure, many of the early commits on this project were not written with the most meaningful messages either! But, then, we improve as we go along.

To revise your commit message, consider following a few best practices like having a short, meaningful subject line separate from the body of your commit message. This greatly helps to see at a glance why a particular commit was made when viewing the commit history when using a command like git log.

There are several sources for advice on this subject. Here is one that popped up on my search today.
https://chris.beams.io/posts/git-commit/

I see that you have submitted a second PR (#55) with an alternate commit message. That is one approach to take--closing a previous PR and submitting a new one after review. However, it is worth noting that you can also modify the existing PR by making changes to your source branch locally and pushing the changes again. This can be done by either adding new commits or modifying existing ones with some like git commit --amend. Sometimes this way can be nice because the discussion history on a particular PR stays in one place, but it really depends on whatever the project members agree on.

If you do opt to go the route of changing git history (git commit --amend, git rebase, etc.), make sure that it is done appropriately. In general, if the commits you are modifying locally have not yet been pushed, you can proceed. If you are the sole owner of a branch (like your patch-1 branch here), it can also be fine to use these tools. However, more caution needs to be taken if you are working with a shared branch and most of the time you shouldn't rewrite commit history in that case (see disclaimers in the Atlassian guide). Also note that if you change history locally for commits already on the GitHub server, you may get a warning message the next time you try to push.

So, to bring your changes into this project, please do one of the following:

  1. Modify your commit message on this PR to conform to best practices OR
  2. Submit a new PR with your revised commit.

Thanks again for your contribution!

@jonathanstallings
Copy link
Copy Markdown
Owner

In case you get this by email, note that I originally included the wrong link for commit message guidelines. Sorry about that. It has been fixed in original comment and linked here.

https://chris.beams.io/posts/git-commit/

@ghost
Copy link
Copy Markdown
Author

ghost commented Oct 15, 2018

Thanks a lot for the links, they're very helpful! Honestly (embarrassingly),
I don't know how to use git yet, so I have been using the GitHub GUI for
everything so far. I did not even mean to fork your project because I
don't even know what that means! Pretty much everything has been
inadvertent thus far. Thank you so much for your patience. I hope that this
next PR is okay :).

@jonathanstallings
Copy link
Copy Markdown
Owner

Closed in favor of #57

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant