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

Resolve context manager issues with Black v23.1.0 formatted tests raising AAA03 #200

Closed
jamescooke opened this issue Feb 15, 2023 · 6 comments · Fixed by #220
Closed

Resolve context manager issues with Black v23.1.0 formatted tests raising AAA03 #200

jamescooke opened this issue Feb 15, 2023 · 6 comments · Fixed by #220
Assignees

Comments

@jamescooke
Copy link
Owner

Problematic code

As of Black 23.1.0 (and possibly earlier versions), the solution of separating the context manager into the Arrange block and keeping the Act block simple won't work because Black now complains about the blank line after the context manager:

def test_with():
    a_class = AClass()
    with freeze_time("2021-02-02 12:00:02"): 

        result = a_class.action('test')

    assert result == 'test'

Black wants to remove the blank link after the context manager and the action, resulting in:

def test_with():
    a_class = AClass()
    with freeze_time("2021-02-02 12:00:02"):
        result = a_class.action("test")

    assert result == "test"

However, current Flake8-AAA raises AAA03 because there is no blank line before the Act block 😞

test.py|4 col 9| AAA03 expected 1 blank line before Act block, found none

Possible solution

One possible solution is to allow for a "greedy context manager" - the freeze_time() context is allowed to join the Act block, even though it's really arrangement:

def test_with():
    a_class = AClass()

    with freeze_time("2021-02-02 12:00:02"):
        result = a_class.action("test")

    assert result == "test"

This passes Black, but would need some fixing on the Flake8-AAA side.

One option would be to create a setting greedy_context_managers or similar which can be set to true for Black users, but I'm concerned that the effort required to test and manage such a setting would be a weight on Flake8-AAA long term.

Greedy context managers were removed in #146 released in 0.10.0, see https://github.com/jamescooke/flake8-aaa/blob/master/CHANGELOG.rst#changed-4

Paper trail

@lyz-code
Copy link

Until this is solved a workaround is to add a comment:

def test_with():
    a_class = AClass()
    with freeze_time("2021-02-02 12:00:02"): 
        # Remove once https://github.com/jamescooke/flake8-aaa/issues/200 is solved

        result = a_class.action('test')

    assert result == 'test'

jamescooke added a commit that referenced this issue Feb 16, 2023
* WIP add problematic and correct code examples

* Add good example of pytest.raises() and update example README

* Build out problematic code / correct code for AAA01

* Fix supported Python versions docs, add Black issue #200

* Update CHANGELOG

* Extend Changelog to contain markers for types of changes
@jamescooke jamescooke self-assigned this Apr 9, 2023
@jamescooke
Copy link
Owner Author

jamescooke commented Apr 9, 2023

Plan Of Action 🏃🏻

I'm hoping that I can roll out a fix across two releases...

Add a setting, next release 0.14.1

  • New setting added for specifying how Act blocks and context managers behave: aaa_act_block_style.
  • Setting default will be 'default'. In this mode context managers that wrap Act blocks remain in the Arrange block, leaving the Act block small:
    def test_with():
        a_class = AClass()
        with freeze_time("2021-02-02 12:00:02"): 
    
            result = a_class.action('test')
    
        assert result == 'test'
  • This way of handling context managers is the current behaviour. Therefore, this release can be rolled out as a patch release.

Add a 'large' option for Act blocks, second release 0.15.0

  • New option added for aaa_act_block_style setting: 'large'.
  • Act blocks get large by absorbing the context managers that wrap them.
    def test_with():
        a_class = AClass()
    
        with freeze_time("2021-02-02 12:00:02"):
            result = a_class.action("test")
    
        assert result == "test"
    Setting this "large" behaviour resolves will resolve the formatting issues with Black.
  • Adding 'large' option to aaa_act_block_style setting won't be a breaking change, so this will be rolled out as a minor version.

Edits

  • 2023/04/11: Prefixed setting with aaa_ so that it becomes aaa_act_block_style. This creates a namespace for Flake8-AAA settings. My guess is that there will be a setting for using result variables called something other than result, e.g. r.
  • 2023/04/13: Commit where fat Act blocks were removed: f9e5edb
  • 2023/04/17: Adjust options to be --aaa-act-block-style=[default|large], i.e. rename "thin" to "default", rename "fat" to "large".

@jamescooke
Copy link
Owner Author

@lyz-code If you have some time to test out master, passing --aaa-act-block-style=large should give you some compatibility with Black. Please let me know how you get on 🙏🏻

I'll try it out with some of my internal projects tomorrow and hopefully write up docs for a release soon, all being well 🤞🏻 .

@lyz-code
Copy link

Hey @jamescooke thanks for the detailed issue evolution, it's very helpful. Sadly I'm a bit overwhelmed these days so sadly I'm not sure if I'll have the time to try it :(.

Thank you so much for developing the fix though <3

@jamescooke
Copy link
Owner Author

Have experimented with the current master on a couple of projects using Black and everything seems to work OK.

Only one caveat: Just formatting the tests with Black is not enough when those tests have context managers. When the context manager has joined the Act block, then there will need to be a blank line added above it to resolve the "AAA03 expected 1 blank line before Act block, found none" errors.

I'll prep release 0.15.0 now.

@lyz-code
Copy link

Thank you so much <3

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 a pull request may close this issue.

2 participants