Conversation
…and git tree problems.
|
Be careful when refactoring. Refactoring explicitly states that functional changes should not be implemented:
Removing winpcap (which is required for wireshark to work correctly, and installed just fine last time I tried) is not refactoring. Revert that please. |
malboxes.py
Outdated
| command = "New-ItemProperty" | ||
| line = "{0} -Path {1} -Name {2} -Value {3} -PropertyType {4}\r\n".format( | ||
| command, args.key, args.name, args.value, args.valuetype) | ||
| line = "{} -Path {} -Name {} -Value {} -PropertyType {}\r\n".format(command, args.key, args.name, args.value, args.valuetype) |
There was a problem hiding this comment.
Mmmm I wonder why I did that. Will fix.
|
I would refrain from doing pure whitelist PEP8 changes except for lines that were above 80 characters. It doesn't help anything regarding lisibility and it makes Otherwise looking good. I'll add Travis-CI to our project and merge. |
malboxes.py
Outdated
| provisioners_list = config["provisioners"][0]["scripts"] | ||
| """ If the script is not already in the profile.""" | ||
| if filename not in provisioners_list: | ||
| provisioners_list.append(fiString) |
There was a problem hiding this comment.
This has never worked. fiString has no other reference. You are not supposed to remove bugs during refactoring 😆
There was a problem hiding this comment.
Fixed in 840ea73 which means that merge won't automatically succeed.
|
What PEP8 changes were not necessary in your opinion ? In my opinion, those helped in the readability of the code so we could be on the same level regarding our coding conventions. For Travis-CI, I think it's a a really good idea ! |
- parser_list = subparsers.add_parser('list', help=
- "Lists available profiles.")
+ parser_list = subparsers.add_parser('list',
+ help="Lists available profiles.")
parser_list.set_defaults(func=list_profiles)Only stuff like this. By default Thoughts? |
|
That seems to be the most popular solution (a vim plugin that has been forked on GitHub) but it needs a vim package manager to be installed so I will personally do that(works great IMO) but I have no problem to accept both styles and not fixing the other lines of code for style-only modifications. |
2 participants
Made some refactoring changes to fix PEP8, code duplicity and git tree problems.