Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extract _build_row_lengths_offsets function #117

Merged
merged 3 commits into from
May 10, 2024

Conversation

ericbn
Copy link
Contributor

@ericbn ericbn commented May 3, 2024

No description provided.

src/ssort/_parsing.py Outdated Show resolved Hide resolved
@ericbn ericbn force-pushed the refactor-split-class branch from 73127cb to 85c7bf6 Compare May 3, 2024 15:33
Copy link
Owner

@bwhmather bwhmather left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for delay. Have difficulty getting free time when I also have internet access.

After much puzzling, I think I've figured out what on earth I was thinking with the original implementation.

I think the missing test case is:

def test_split_class_with_bases():
    actual = _split_class("class A(\n    B,\n):pass")
    expected = "class A(\n    B,\n):", ["    pass"]
    assert actual == expected

Here the line with class... on it is not the same as the line that ends the class head.

With this change we get:

'class A(', ['    B,\n):pass']

Sadly I don't think there is a node for the trailing bracket or colon so I don't know what can be done.

@ericbn ericbn force-pushed the refactor-split-class branch from 59e2036 to 51296f0 Compare May 10, 2024 13:39
@ericbn ericbn changed the title Refactor split_class in _parsing.py Extract _build_row_lengths_offsets function May 10, 2024
@ericbn
Copy link
Contributor Author

ericbn commented May 10, 2024

I totally appreciate your patience in reviewing this and not letting me almost mess it up! :- )

I've updated the PR with a smaller refactoring, adding the test case that you mentioned and also fixing the recent error with the coverage check.

@@ -30,6 +30,10 @@
def _nodes_types(
node_type: type[ast.AST] = ast.AST,
) -> Iterable[type[ast.AST]]:
# coverage package adds a coverage.parser.NodeList subclass
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's just naughty. Why on earth does it do that?

@bwhmather
Copy link
Owner

Thanks. Mind restoring the two test cases you had in before? They looked worthwhile.

ericbn added 2 commits May 10, 2024 11:28
for the more complex cases that cover the implementation using tokenize.
Was failing with

    tests/test_ast.py:71: in <module>
        @parametrize_nodes()
    tests/test_ast.py:60: in parametrize_nodes
        nodes = [_instantiate_node(node_type) for node_type in node_types]
    tests/test_ast.py:55: in _instantiate_node
        return node_type(*([""] * len(node_type._fields)))
    E   TypeError: NodeList.__init__() missing 1 required positional argument: 'body'

coverage package adds a coverage.parser.NodeList subclass. See
nedbat/coveragepy@0accb68
@ericbn ericbn force-pushed the refactor-split-class branch from 51296f0 to 51354eb Compare May 10, 2024 16:31
@ericbn
Copy link
Contributor Author

ericbn commented May 10, 2024

Thanks. Mind restoring the two test cases you had in before? They looked worthwhile.

Sure, done!

@coveralls
Copy link

Pull Request Test Coverage Report for Build 9035218786

Details

  • 11 of 11 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.004%) to 98.257%

Totals Coverage Status
Change from base Build 8749000911: -0.004%
Covered Lines: 1353
Relevant Lines: 1377

💛 - Coveralls

Copy link
Owner

@bwhmather bwhmather left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much. Very happy to take any other improvements that might tickle your fancy.

@bwhmather bwhmather merged commit ca350f4 into bwhmather:master May 10, 2024
22 checks passed
@ericbn ericbn deleted the refactor-split-class branch May 10, 2024 19:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants