Add track command #2
Conversation
williamjamir
force-pushed
the
fb-add-track-command
branch
3 times, most recently
from
04d4bff to
7f70d2b
Compare
williamjamir
force-pushed
the
fb-add-track-command
branch
from
7f70d2b to
a1d85a3
Compare
| @@ -1,3 +1,5 @@ | |||
| skip_branch_with_pr: true | |||
There was a problem hiding this comment.
Hmm this is interesting. Does travis has a similar option?
There was a problem hiding this comment.
I didn't find a "Skip branch" on Travis to avoid duplication, but I could find the following snippet that enables a "whitelist". So, Travis will just create jobs for master and Pull Requests to master.
branches:
only:
- "master"
appveyor.yml
Outdated
| TOX_ENV: "py36" | ||
|
|
||
| global: | ||
| LOGNAME: "LOGNAME" |
There was a problem hiding this comment.
This seems like a placeholder that was left in there
There was a problem hiding this comment.
Actually, this was on purpose because of GitPython. A better approach was taken on commit 0070645 . The file tox.ini has comments and links explaining the issue that these variables solve.
- Task: Remove global env
module_renamer/cli.py
Outdated
| return 0 | ||
| @main.command() | ||
| @click.argument('project_path', type=click.Path(exists=True)) | ||
| @click.option('--origin_branch', default='master', |
There was a problem hiding this comment.
Options usually are separated by dashes: --origin-branch, same for --work-branch.
Also, a more intuitive command-line IMO would be:
module-renamer --output-file=foo.py # current branch (with modifications) compared to "master"
module-renamer --branch=my-branch --compare-with=master --output-file=foo.py # "my-branch" with modifications compared with "master"
There was a problem hiding this comment.
I was not using the dashes because I was not being able to access it, but I just found that any internal " - " characters will be converted to underscore.
The next commit improves this inputs ;)
- Fixed on commit d6b11ea
| """ | ||
| Change the current git branch for the branch informed on branch_name | ||
| :param repo: A git.Repo object that represents the git repository from the project |
There was a problem hiding this comment.
As we discussed, please configure the line length to 100 👍
| FormatFile(file_name, in_place=True) | ||
|
|
||
|
|
||
| def generate_list_with_modified_imports(origin_import_list, working_import_list): |
There was a problem hiding this comment.
Hmm this is complaining about the length of the function: len("generate_list_with_modified_imports") == 35. Please see if this is customizable as well (I don't think we should add an upper limit to the name of the function, but enforcing it to be all lower-case is good)
|
|
||
| with open(file_name, 'w') as file: | ||
| file.write("imports_to_move = ") | ||
| file.writelines(repr(list(list_with_modified_imports))) |
There was a problem hiding this comment.
Consider using pprint instead to write a more readable file.
There was a problem hiding this comment.
- Fixed on commit d6b11ea
| file.write("imports_to_move = ") | ||
| file.writelines(repr(list(list_with_modified_imports))) | ||
|
|
||
| FormatFile(file_name, in_place=True) |
There was a problem hiding this comment.
Hmm why is this necessary? I mean, the file is temporary and will not be committed to the repository right?
There was a problem hiding this comment.
This is the actual file that the scripts generate (to be used later on). FormatFile it's a function from YAPF to write a readable file.
I didn't know the existence of pprint, before your review so I changed FormatFile to pprint =)
- Fixed on commit d6b11ea
| if confirm(INFORMATIVE_CONFLICT_MSG, abort=True): | ||
| list_with_modified_imports = [modified_import for modified_import in | ||
| list_with_modified_imports | ||
| if modified_import[0] not in imports_with_conflict |
There was a problem hiding this comment.
Each one of these ifs is a linear search, depending on the number of imports this might impact the performance.
| Look for all .py files on the given project path and return the import statements found on | ||
| each file. | ||
| Note.: I inserted the TQDM here because was the only way that I could have an accurate |
There was a problem hiding this comment.
It seems the handling of TQDM could be moved to the calling code given that this function yields each import it finds...
There was a problem hiding this comment.
Sure, but the progress is based on the total number of files since I cannot know how many imports each file has beforehand.
| yield os.path.abspath(os.path.join(dir_path, filename)) | ||
|
|
||
|
|
||
| def total_of_py_files_on_project(project_path): |
There was a problem hiding this comment.
As I said previously, it seems like a waste to recursively iterate over all files and then throw the result away.
There was a problem hiding this comment.
- Fixed on commit d6b11ea
Codecov Report
@@ Coverage Diff @@
## master #2 +/- ##
==========================================
+ Coverage 71.42% 90.9% +19.48%
==========================================
Files 5 6 +1
Lines 35 187 +152
==========================================
+ Hits 25 170 +145
- Misses 10 17 +7
Continue to review full report at Codecov.
|
|
The inspection completed: 13 new issues, 24 updated code elements |
williamjamir
force-pushed
the
fb-add-track-command
branch
from
e377f4e to
77e238b
Compare
williamjamir
force-pushed
the
fb-add-track-command
branch
from
77e238b to
d6b11ea
Compare
Add track command for the rename script, with this command the user can compare the difference between two branches and generate a list of the imports that was changed.