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

timezone-aware datetime Python 3.11+ compatible formatting support #159

Open
MRigal opened this issue Dec 19, 2024 · 4 comments
Open

timezone-aware datetime Python 3.11+ compatible formatting support #159

MRigal opened this issue Dec 19, 2024 · 4 comments

Comments

@MRigal
Copy link

MRigal commented Dec 19, 2024

Hi @15r10nk

Thanks for the great library! This may be another "proof" that support formatting through ruff #58 would be great!

I use ruff to lint/format my codebase and until now it did not create any issue. However, I've enabled timezone-aware datetime in all my models and columns and enabled the great flake8-datetimez set of rules. I'm formatting my code against Python 3.12 as I like to have modern Python code and here comes the problem.

inline-snapshot via black and its support for timezone-aware datetime creates output similar to:

"updated_at": datetime.datetime(2024, 6, 18, 20, 19, 42, tzinfo=datetime.timezone.utc)

which gets formatted through ruff to, via UP017:

"updated_at": datetime.datetime(2024, 6, 18, 20, 19, 42, tzinfo=datetime.UTC)

Thus, the snapshots keep on failing. Any idea how I could fix that? I can add a # noqa: UP017 to the line or disable it for tests, but both do not feel so good and it introduces a style mismatch in the codebase.

I may contribute to a fix, but if it is "implement ruff support", I guess it's too big for me! :-D

@15r10nk
Copy link
Owner

15r10nk commented Dec 19, 2024

I would not say that this what ruff does here is formatting any more, but it is still an issue and thank you for bringing it up.

─────────────────────────────────── Update snapshots ───────────────────────────────────
╭────────────────────────────────── ruff_lint_bug.py ──────────────────────────────────╮
│ @@ -4,5 +4,5 @@                                                                      │
│                                                                                      │
│                                                                                      │
│  def test():                                                                         │
│      assert datetime.datetime(2024, 6, 18, 20, 19, 42, tzinfo=datetime.UTC) == snaps │
│ -        datetime.datetime(2024, 6, 18, 20, 19, 42, tzinfo=datetime.UTC)             │
│ +        datetime.datetime(2024, 6, 18, 20, 19, 42, tzinfo=datetime.timezone.utc)    │
│      )                                                                               │
╰──────────────────────────────────────────────────────────────────────────────────────╯

let me give you an example why the current behavior exists.

from inline_snapshot import snapshot
from dataclasses import dataclass

class NewClass:
    pass

OldClz=NewClass

def something():
    return NewClass


def test():
    assert something() == snapshot(OldClz)

inline-snapshot changes the name of the class to the new representation because this is how the object should now be represented.

─────────────────────────────────── Update snapshots ───────────────────────────────────
╭───────────────────────────────────── update.py ──────────────────────────────────────╮
│ @@ -13,6 +13,6 @@                                                                    │
│                                                                                      │
│                                                                                      │
│                                                                                      │
│  def test():                                                                         │
│ -    assert something() == snapshot(OldClz)                                          │
│ +    assert something() == snapshot(NewClass)                                        │
╰──────────────────────────────────────────────────────────────────────────────────────╯

The reason for such updates could also be changes in the __repr__() implementation of the object, which have no effect to the value (omit default arguments for example).

Your problem is that the new name is an alias for the old name, and not the other way around.

I have to think about this a bit more. Formatting alone might not help because inline-snapshot will tell you that there are some snapshots which can be updated, but there will be no change when the updates are applied (because ruff reverted these changes). This can be confusing for the user.

One solution might be to split the update category into two. One for changes which are triggered by inline-snapshot (like changes in the multiline string representation). And another for all other changes.

But the question is then: How do I present these other changes to the user. Don't show them because they are confusing/wrong (your case) or show them because they are useful (new repr).
That's the problem where we started.

@MRigal
Copy link
Author

MRigal commented Dec 20, 2024

Hi @15r10nk !

Thanks for your detailed answer! I had understood the implementation and considerations leading to the current behavior, but exposing them so clearly may help someone to come with a good idea.

I completely share that it's not gonna be easy to decide which changes is worth presenting or not and I don't have a smart hint to add here...

On the other hand, if inline-snapshot could support/enable native ruff formatting, using the rules present in the project, we would not have that issue anymore, right? Although it may still happen on a black + pyupgrade lint/formatting setup...

Have you considered hooking on pre-commit else? Could that be a suitable config to support a wide variety of setups?

@15r10nk
Copy link
Owner

15r10nk commented Dec 20, 2024

Formatting has two goals:

  • I want to format the snapshots because they would otherwise just be really long strings
  • I don't want that the formatting changes existing code.

Doing more than formatting (pre-commit) is not something which I would like to do here. But inline-snapshot should not try to "fight" with other tools.

I don't think that formatting is a solution to this problem, because inline-snapshot would still think that the code should be changed (notify the user ... 3 snapshots to update), but nothing would be changed when the updates are performed. Which would confuse the user because inline-snapshot still tells him that it wants to change something.

The best solution I have currently is to restrict the update changes to a explicit set of change types (string formatting, default arguments, ...) but not arbitrary changes in the representation.

Maybe put them behind a question like:

Do you want to test for changed repr() (y/N)

I guess the situation might be a different one if you don't see these changes every time you run --inline-snapshot=review, am I right?

@MRigal
Copy link
Author

MRigal commented Dec 20, 2024

That's true, but as a user, I'm not sure how I would react to a quite generic question like:

Do you want to test for changed repr() (y/N)

I may press N because I might fear to miss other more problematic changes...

Would maybe some kind of config/mapper for equivalences/replacements in the repr() be an option? To make it specific and transparent enough?

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

No branches or pull requests

2 participants