make CLI arguments more concise

[samuelcolvin]

I'm using inline-snapshots on all projects I'm working on (I just added it to pydantic-core, pydantic/pydantic-core#1512).

As I use it more and more, there are 3 related things that I find a bit annoying:

1. The CLI arguments like --inline-snapshot=fix and --inline-snapshot=create are quite verbose to write out when I want to enable them, could we add --snap-fix etc?
2. --inline-snapshot=create isn't on by default and --inline-snapshot=fix doesn't imply --inline-snapshot=create - could you add a config setting or env var to have --inline-snapshot=create on by default? and/or could --inline-snapshot=fix imply --inline-snapshot=create?
3. When I enable --inline-snapshot=fix or --inline-snapshot=create I get loads of diffs from tests that are passing but where snapshot values are formatted differently to inline-snapshots default formatting. I've seen this confuse some of our team a lot, as they thought these tests were failing, I think the diff should be hidden for tests that are passing by default.

Sorry if these should be three separate issues, I thought there were related enough to put together. Happy to split if you'd prefer.

[15r10nk]

Thank you for this feedback. I always like to know how other people use inline-snapshot with different workflows like mine.

--inline-snapshot=fix and --inline-snapshot=create are quite verbose

I agree. I use usually --inline-snapshot=review which allows me to see the the changes before I apply them to my code base (but this would currently not be ideal in the logfire codebase ...).

In the future I want to move towards a workflow where inline-snapshot shows you the changes and allows you to apply them after you approved them. The categories fix,trim,create,update are useful information for the developer why something changed.

--inline-snapshot=create isn't on by default

There are two reasons.

1. every inline-snapshot option (except disable/short-report) disables the assert rewriting of pytest. I think this would not be a good idea by default. However I have a plan to change this in the future.

2. I also don't wanted to enable any source code rewriting by default. This might also affect every normal pytest run and could have unexpected consequences. But you can make it to your default in the config or with a env variable

I get loads of diffs from tests that are passing but where snapshot values are formatted differently to inline-snapshots default formatting. I've seen this confuse some of our team a lot, as they thought these tests were failing, I think the diff should be hidden for tests that are passing by default.

Logfire is really useful for me here because it shows me how people are using inline-snapshot. The problem is that the developer replaces sometimes parts of the snapshots with other expressions (usually for good reasons), but inline-snapshot wants to control everything inside snapshot(...) (except DirtyEquals).
I currently work on a solution where the developer can use Is(...) inside a snapshot to take back control. This leads to a kind of holes within the snapshot.

example from logfire where the redis port is changed by the developer:

    assert exporter.exported_spans_as_dict() == snapshot(
        [
            {
                'name': 'SET',
                'context': {'trace_id': 1, 'span_id': 1, 'is_remote': False},
                'parent': None,
                'start_time': 1000000000,
                'end_time': 2000000000,
                'attributes': {
                    'logfire.span_type': 'span',
                    'logfire.msg': 'SET ? ?',
                    'db.statement': 'SET ? ?',
                    'db.system': 'redis',
                    'db.redis.database_index': 0,
                    'net.peer.name': 'localhost',
                    'net.peer.port': Is(redis_port),
                    'net.transport': 'ip_tcp',
                    'db.redis.args_length': 3,
                },
            }
        ]
    )

here are some more changes if you are interrested (all still wip).

[alexmojaki]

Parts 1 and 2 sound a lot to me like #57, #62, and #109. I want to just run tests my usual way through pycharm and have it always fix snapshots locally without extra interaction, but i want it to tell me (by making the tests fail after the fact) which tests needed fixing. It sounds like others feel similarly. I don't want something where I have to approve changes in an interface managed by inline-snapshot.

For 3, we shouldn't have to use Is() on every non-literal expression. Why is that report shown by default? I've never wanted to use update or be told what would have happened if I'd used it.

[samuelcolvin]

What I did in insert_assert in devtools was fail any test where we did a rewrite.

See here

pytest.fail(f'devtools-insert-assert: {count} assert{plural(count)} will be inserted', pytrace=False)

But do the rewrite by default. I think it would be great to have a way to do this on inline-snapshot=create and have it enabled by default, or via an env var.

[samuelcolvin]

Using

export INLINE_SNAPSHOT_DEFAULT_FLAGS=create

is a good start, although I still think it's unfortunate that it causes all the review diffs to be printed.

(BTW, I'm mostly using inline-snapshots on a library I haven't open sourced yet, I will do fairly soon, which might provide more context)

[15r10nk]

but i want it to tell me (by making the tests fail after the fact) which tests needed fixing

Will also be implemented.

You can already fix and create all snapshots by default with

export INLINE_SNAPSHOT_DEFAULT_FLAGS=create,fix

I don't want something where I have to approve changes in an interface managed by inline-snapshot.

You don't have to use it. I want to support different workflows, but i will never change code by default without user interaction. There will be ways (like the above) which will allow you to configure inline-snapshot in the way you like.

For 3, we shouldn't have to use Is() on every non-literal expression. Why is that report shown by default? I've never wanted to use update or be told what would have happened if I'd used it.

The update category is for code changes which cause no test failure. It was used in the past to change multi line string representation like

"line1\nline2\n"

into

"""\
line1
line2
"""

and there might be other changes in the future where I change the way inline-snapshot represents the values.

It is also used when __repr__() of an type is changed and returns an different result.

I separate this from fix to let the developer know that the values have not been changed, which simplifies the approval.

inline-snapshot was never designed to support snapshot changes by the developer. The logfire code surprised me here in a nice way because you explored some new ways to use snapshots.

But the inline-snapshot will not only try to update these snapshots to the current repr() of the value but also fix it if the value changes and discard any code which was written by the developer.

Is(...) should make clear that the developer is responsible for this code. inline-snapshot will not update it or fix it if the value is not equal.
It is also useful if you read/review the code because you know which part was written by inline-snapshot and which by the developer.

[alexmojaki]

Is() will block fix? not sure if i like that

[15r10nk]

Is() will block fix? not sure if i like that

Can you give an example where you want that inline-snapshot replace code which you have written?

[alexmojaki]

1. It could simply be that the values have actually changed and the test needs updating. I don't have examples and it may not be likely, but it sounds like if that happens then I have to manually do what inline-snapshot is supposed to automate for me. 2. I'm not sure what the semantics are when the value doesn't match now. Does the assertion just fail? Either way it sounds harder to think about. 3. I would usually rather see the diff in git (as a result of the snapshot changing) than in the pytest failure, so I'd often prefer to apply the snapshot change even if it's temporary. 4. I generally find it obvious to see whether a value was written by inline-snapshot or not. 5. I do not want to have to insert `Is()` wherever appropriate. I just don't want to see the report about hypothetically using `update`. I still don't think it should show by default, but if for some reason you want to keep it, then I'd like it to be easier to turn it off. It looks like I can use `--inline-snapshot=fix,short-report`. I think `INLINE_SNAPSHOT_DEFAULT_FLAGS=short-report pytest --inline-snapshot=fix` should work, by combining the flags, but it seems it doesn't, so I can't just put `INLINE_SNAPSHOT_DEFAULT_FLAGS=short-report` in my `.zshrc` or something.

…

On Tue, Nov 5, 2024 at 11:34 AM Frank Hoffmann ***@***.***> wrote: Is() will block fix? not sure if i like that Can you give an example where you want that inline-snapshot replace code which you have written? — Reply to this email directly, view it on GitHub <#123 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA3VTWOOEVW5GRDP26REKVDZ7CGJ5AVCNFSM6AAAAABRDYXOYWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINJWGY3TMMBXGQ> . You are receiving this because you commented.Message ID: ***@***.***>

[samuelcolvin]

I think this conversation is too wide ranging, that's entirely my fault for creating an issue with multiple points.

I think we should close this, and I'll create individual issues for the things I would love to change.

[samuelcolvin]

Closing in favour of #127 and #128. @alexmojaki if you have other points, best to create new issues.