This repository has been archived by the owner on Apr 4, 2023. It is now read-only.
Several Pylint suggestions #332
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We removed this functionality when switching to the database-based approach, but missed this command (which we don't generally call). H/T to pylint for pointing it out
We were testing the result of the generator, which is a constant, thereby doing more processing than was necessary. Another H/T to pylint
These should be backwards-compatible. In most cases, they are just swapping variable names to prevent scoping issues or converting a method into a static one, but in a few, we're accounting for dynamic corner cases we never encountered in practice -- yet.
These include avoiding [] for default params, switching to static methods, avoiding overriding method names, and some indentation issues.
We had been setting a bunch of instance vars in the `process` function, which isn't always called. Set them on __init__ instead.
This makes the tested logic much easier to understand
Mostly converting methods to static, renaming variables, and using raw strings
| """Position in match reflects XML tags, so its dropped in | ||
| preference of new values based on node.text.""" | ||
| # Position in match reflects XML tags, so its dropped in | ||
| # preference of new values based on node.text.""" |
There was a problem hiding this comment.
Very minor, but we probably want to lose the trailing """ here.
| @@ -236,7 +236,7 @@ def match_data(self, match): | |||
|
|
|||
|
|
|||
| class Footnotes(PlaintextFormatData): | |||
| """E.g. [^4](Contents of footnote with escaped parens: \(\)s)""" | |||
| r"""E.g. [^4](Contents of footnote with escaped parens: \(\)s)""" | |||
There was a problem hiding this comment.
? Do we want to mark this comment as regex text?
There was a problem hiding this comment.
The r prefix just means "raw", so we don't need to double-escape the slashes. If we don't use an r, we're technically encoding bad escape sequences (\( and \)). What should we do? I'd like to include the indication that parens can be escaped, but not if it makes the code harder to understand.
There was a problem hiding this comment.
I might not be understanding this correctly, but I would consider trying something like:
"""
Handles footnotes.
A variable with text that would match looks something like this::
fn = r"[^4](Contents of footnote with escaped parens: \(\)s)"
"""
Thanks for catching this @tadhg-ohiggins!
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Using pylint as a guide, I went through and fixed up lots of little odds and ends. For the most part, these were little things like indentation, variable names overriding functions, unused arguments, etc., but there's a handful of more important changes. Each of the
[]default args was a potential bug; in one case we were testing the presence of a generator (always true); in a third, the SxS command hadn't been updated (pylint pointed out that we were using a non-iterable object as an iterable). There's a lot here, but it's all low risk. Easiest to review by changeset.