-
Notifications
You must be signed in to change notification settings - Fork 16
Issue #15: add the function to show all blame output. #16
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
I like the approach, currently is the only way to remove the phantom from the right click context menu? Either way im happy to merge regardless of your answer, just let me know. |
…ode in the `close` command.
@psykzz thank you very much for your kind reply :)
Currently there are 2 ways to remove the phantoms: 1) edit the file, 2) select
I tried adding |
Looks good to me, will merge and release later today. |
One other thing @gh640 could you update the readme too with the new commands? |
@gh640 I checked out your branch and tested the new commands with a couple of files in several projects. For the most part it works well, and I think the layout shows a good amount of information without taking up too much space. One caveat, though: On very large files (for example, a CSS file with > 1200 lines), the command doesn't appear to do anything. Not sure if this is due to API limits or an issue with this implementation (I didn't have time to look at your code yet). |
@Tardog thank you for testing the branch!
I believe it's caused since I didn't properly consider the leading caret of commit hash (SHA). I fixed it in a new commit. Also, I found a problem with my PR which breaks the existing function with my mistake of renaming a variable name :( I fixed it in another new commit. Could you please try the new HEAD? Thank you. |
FYI, I noticed that Sublime Text almost freezes if I use this new command for a file with over 10,000 lines in my environment... (This happens with the existing git blame command with over 10,000 selections, too.) In the new command, the |
Ill do another review once @Tardog has had a look into if your changes fixes their issue. |
@gh640 Wow, that was fast! I’ve looked at the changes in your latest commits and pulled them for another round of testing. There are still no phantoms showing up in the large CSS file. Ditto for a much smaller (~500 lines) Python file. In order to help debugging this issue a little better, I disabled all other packages (just to make sure I can test your changes in isolation), and looked at the git blame output of the affected files. There is no leading caret in any of their commit hashes – although it was quick thinking on your part to anticipate this scenario and change the regex accordingly. :) If I find a file with boundary commits, I'm going to make sure to test it as well. So, what else could be preventing the phantoms from showing up? No errors in the ST console, no slowdown when executing the "Show all" command, and the single-line blame works flawlessly. I started editing the problematic Python file, removing parts of the code and executing "Git Blame: Show all" after each modification. It quickly turned out that even with only a few lines left in the file, the phantoms were still AWOL. Not a matter of size, then? After comparing git blame output of the troublesome Python and CSS files, I noticed both have several commits from authors with special characters in their names. I think these are causing the command to fail. To make this easier for you to reproduce, here is the Python file I used for testing: https://github.com/DragonComputer/Dragonfire/blob/master/dragonfire/sr/experimental.py Run the command, see it fail. try:
raw_input # Python 2
except NameError:
raw_input = input # Python 3
try:
xrange # Python 2
except NameError:
xrange = range # Python 3 Run the command again. Phantoms should pop up now. Thank you for putting in the effort to get this feature up and running! I'll gladly help and continue testing with as many different files as possible. |
…e_show_all" command.
@Tardog thank you very much for addressing the problem. Your thorough tests gave me lots of hints! Firstly I could reproduce the phenomenon with the file you shared: https://github.com/DragonComputer/Dragonfire/blob/master/dragonfire/sr/experimental.py I found a cause which made the command fail silently. It's the file name in the blame output. I didn't properly recognize the pattern that blame result has file names. (As psykzz knows) the file names are displayed in blame result only when the file name was changed in the commit history. I didn't understand that behavior of I adjusted the code a little in the new commit to make it work. The I'd like you to check and review it out again :) |
(I'd like to propose a debug option to make debugging easier in this package later) |
I second this. A debug option would be superb. With the latest changes I get the expected output for all the files on which the command failed before. Looks good to me! So far I haven't been able to find a project with boundary commits to test. Can someone help out with this? @gh640 I could have sworn it was the special characters. :D Turns out I didn't know about the optional file name display in |
@Tardog thank you for your swift response! I'm happy to hear that.
I myself had a file with boundary commits and tested it. The command seems to work well for the file.
Yeah. It was a good learning for me, too :D As for the debug option, I'll give it a try after we've completed this issue. Thanks! |
@psykzz I'd like you to review the PR when you have a time as Tardog finished to review it. Thank you in advance. |
Merged and released as version |
Closes #15
Thank you for the great package. This package helps me a lot.
I'd like to propose an approach to realize the multi-line blame display function.
Here is a sample screenshot.