Skip to content

Conversation

@Pierre-Sassoulas
Copy link
Collaborator

This will permit to reuse the code to assert that a formatter does something if activated or nothing if deactivated more easily.

@coveralls
Copy link

coveralls commented Jan 9, 2022

Pull Request Test Coverage Report for Build 1780515459

  • 47 of 47 (100.0%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.1%) to 99.656%

Totals Coverage Status
Change from base Build 1671482333: 0.1%
Covered Lines: 290
Relevant Lines: 291

💛 - Coveralls

@Pierre-Sassoulas
Copy link
Collaborator Author

Pierre-Sassoulas commented Jan 9, 2022

Permit to do #30 's test with:

def test_optional_formatters_argument(
    capsys: pytest.CaptureFixture[str],
    tmp_path: Path,
) -> None:
    """Test that an optional formatter is correctly turned on and off with arguments."""
    with FormatterAssert(
        '"""Summary. Body."""', [SplitSummaryAndDocstringFormatter()], capsys, tmp_path
    ) as asserter:
        asserter.assert_format_when_activated()
        asserter.assert_no_change_when_deactivated()

@DanielNoord
Copy link
Owner

Just two general comments:

  1. I think we should make everything in testutils private with a leading underscore for now. (As long as we don't support plugins nobody would need it anyway).
  2. Is there any use case for those properties outside of testing? If not I think it would be better to make them into utility functions within the testutils package. Not really a need to add functions to the codebase that are only necessary for testing imo.

@DanielNoord
Copy link
Owner

@Pierre-Sassoulas Have you had time to look at this?

@Pierre-Sassoulas
Copy link
Collaborator Author

No sorry I have a pretty busy week, it's on my radar though.

if not I think it would be better to make them into utility functions within the testutils package.

Did you mean a function inside tests ?

@DanielNoord
Copy link
Owner

No sorry I have a pretty busy week, it's on my radar though.

No rush!

if not I think it would be better to make them into utility functions within the testutils package.

Did you mean a function inside tests ?

I think in testutils. Like: if we don't need those Class properties for the normal functionality of the program I think we shouldn't put them on the class (as they impact runtime of the program). So everything that is only necessary to test the program should be in testutils so it only impacts the running of the test suite instead of the actual runtime of the program.

@Pierre-Sassoulas
Copy link
Collaborator Author

Allright I understand.

Like: if we don't need those Class properties for the normal functionality of the program

We could refactor the argparsing to use them (I did not do that yet.) Maybe we could do the refactor first ? what do you think ?

@DanielNoord
Copy link
Owner

Allright I understand.

Like: if we don't need those Class properties for the normal functionality of the program

We could refactor the argparsing to use them (I did not do that yet.) Maybe we could do the refactor first ? what do you think ?

Yeah makes sense. I'll do a final check of this PR > update the private names > merge and do a follow-up PR to argparse.

@DanielNoord DanielNoord merged commit 607999b into main Feb 1, 2022
@DanielNoord DanielNoord deleted the add-testutil-context-manager branch February 1, 2022 20:21
@DanielNoord
Copy link
Owner

Follow up in #37.

@Pierre-Sassoulas
Copy link
Collaborator Author

Thank you for finalizing this for me 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request maintenance

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants