Switch to yamf for lint. #272
Conversation
| results = { | ||
| frozenset(pair): self.get_pair_compatibility(pair) | ||
| for pair in package_pairs | ||
| } |
There was a problem hiding this comment.
For a linter that supposedly only accepts one way of doing, it seems like this would be an consistency. package_pairs and results both are list/dict comprehensions, but the braces for results are buffered by new lines while that doesn't happen for package_pairs.
I realize that you probably ran the yamf linter and applied a suggested changes in this PR, and I'm not totally familiar with google style, but this seems weird to me, so I wonder if there's a clear a reason for this?
There was a problem hiding this comment.
The style guidelines are silent on how to format in these cases. yapf has a cost associated with line splitting but I'm not sure of the exact weight.
| return msg.format(self.name, self.priority.level.name, | ||
| self.installed_version, self.latest_version, | ||
| (self.current_time - self.latest_version_time).days, | ||
| self.priority.details) |
There was a problem hiding this comment.
It seems like this linter expands and contracts the spacing in random places. Here, (and I don't know if google style mandates this way of linting or not, but) I personally think the original code was easier to read since each line contained a distinct argument whereas in the change, it's become very condensed and you have stop and read carefully since not only are there are more than 1 argument per line, there are periods and comas all over the place.
There was a problem hiding this comment.
It assigns a cost to line breaks and tries to avoid them. We need to decide whether this tool makes things better overall. If it doesn't then I should just revert this PR.
There was a problem hiding this comment.
Overall, I think yapf does a better job than flake, so I'm on board for using this tool.
I just question some of the knobs(?) used in the current configuration. I was skimming some of the documentation and am thinking that maybe by turning some of these knobs on/off, we can get more consistency/better readability: COALESCE_BRACKETS, DEDENT_CLOSING_BRACKETS, SPLIT_ALL_COMMA_SEPARATED_VALUES
| content_dict = ast.literal_eval( | ||
| content.replace('true', '"true"').replace('false', | ||
| '"false"').replace( | ||
| 'null', '"null"')) |
There was a problem hiding this comment.
It could just be that this one line is trying to do too much and should actually be broken up, but it just looks really weird at the moment.
| @@ -378,7 +367,8 @@ def main(): | |||
| process.expect('Ready for new connection', timeout=5) | |||
|
|
|||
| packages = [ | |||
| package.Package(install_name) for install_name in args.packages] | |||
| package.Package(install_name) for install_name in args.packages | |||
| ] | |||
There was a problem hiding this comment.
It's interesting that the brackets are buffered by new lines in this list comprehension definition.
| @@ -260,7 +254,7 @@ def _run_container(self, | |||
| self._mmap.record(self._tmap) | |||
| raise PipCheckerError( | |||
| error_msg="An error occurred while starting a docker " | |||
| "container. Error message: {}".format(e.explanation)) | |||
| "container. Error message: {}".format(e.explanation)) | |||
There was a problem hiding this comment.
I think this would be weird - it makes more sense to use parens
error_msg=("An error occurred while starting a docker "
"container. Error message: {}").format(e.explanation))
or break that line up by defining error_msg on a separate line beforehand.
There was a problem hiding this comment.
Actually, parens were required before: http://google.github.io/styleguide/pyguide.html#32-line-length
I can fix this everywhere but first we should decide whether we want to do this at all.
| "out. Error msg: {}".format( | ||
| command, returncode - 128, output)) | ||
| "This likely means that the Docker container timed " | ||
| "out. Error msg: {}".format(command, returncode - 128, output)) |
| @@ -477,11 +465,10 @@ def _check(self, container: docker.models.containers.Container): | |||
| if not has_version_conflicts: | |||
| raise PipCheckerError( | |||
| error_msg="The docker container timed out before executing" | |||
| "pip command. Error msg: {}".format(output)) | |||
| "pip command. Error msg: {}".format(output)) | |||
| compatibility_store.Status.SUCCESS.name, | ||
| 'self': | ||
| True, | ||
| }) |
| compatibility_store.Status.UNKNOWN.name, | ||
| 'self': | ||
| False, | ||
| }) |
| compatibility_store.Status.SUCCESS.name, | ||
| 'self': | ||
| False, | ||
| }) |
No description provided.