Update/fix diff #13
Update/fix diff #13
Conversation
josix
commented
Jun 7, 2018
- Update git show to take an arbitrary hash
- Add current working directory to file path
* Add original_po_file_commit_hash to main func argument * Add argument <original_po_file_commit_hash> to argv
* Avoid a path checking thrown by git show (will lead to crash)
scripts/fix_diffs.py
Outdated
| def main(fp): | ||
| p = subprocess.Popen(['git', 'show', 'HEAD:' + fp], stdout=subprocess.PIPE) | ||
| def main(fp, original_po_commit='HEAD'): | ||
| p = subprocess.Popen(['git', 'show', '{}:'.format(original_po_commit) + fp], stdout=subprocess.PIPE) |
There was a problem hiding this comment.
I'd propose to use string formatting for both original_po_commit and fp instead of using both formatting and concatenation.
Or actually, since this is the Python 3.6 translation, why don't we use f-strings? 😎
There was a problem hiding this comment.
Oh, that's right! I will update it into using f-string.
scripts/fix_diffs.py
Outdated
| if len(sys.argv) < 2: | ||
| print('Usage: python fix_diffs.py <po_file_path>') | ||
| print('Usage: python fix_diffs.py <po_file_path> <original_po_file_commit_hash>') |
There was a problem hiding this comment.
We should mark this argument as optional through marking the variable with a pair of square brackets or something similar. And also it would be great to show the default value (which is HEAD) here in some way.
Also I'd suggest to not use the word "hash" here as it can be HEAD, HEAD~3 etc. which are not hashes but represents a commit.
There was a problem hiding this comment.
Thanks for your suggestion! I've fixed it. Please check it whether proper.
scripts/fix_diffs.py
Outdated
|
|
||
| fp = sys.argv[1] | ||
| output_lines = main(fp) | ||
| fp = './' + sys.argv[1] |
There was a problem hiding this comment.
I'm curious about this line, can you please explain the reason behind this line of code? As far as I understand, a relative path defaults to be relative to the current directory. What is an example case you're trying to resolve with this?
There was a problem hiding this comment.
I found an error when I was running the script in the subdirectory 'tutorial' (or any subdirectory like '/reference',' /howto'...etc):
(Actually, I don't know how to explain this bug in detail 😅. Maybe the image below could reveal this bug.)
There was a problem hiding this comment.
I see where the problem is, this is because I was passing the filename argument directly to git show, however the git object indicating the file at HEAD should be HEAD:<path> where the path argument is by default relative to the root of the git repo.
Thanks! ✨
|
@wilson8507 Thanks for contributing! I've left some comments here, it would be great if you can take a look at them 🙏 |
* Update argument naming * Add exit against wrong arguments input
|
@adrianliaw I've fixed the problems mentioned above, please check it ~ Thank you! |