jamescooke/flake8-aaa

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

jamescooke opened this issue ยท 6 comments

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

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'

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".

@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 ๐Ÿคž๐Ÿป .

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

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.

Thank you so much <3