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

templates/package: fix template to use triple quotes #175

Merged
merged 3 commits into from
Apr 15, 2024

Conversation

BurntSushi
Copy link
Member

I noticed that when using a requirement like

c<2.0.0 ; sys_platform == 'linux'

then the single quotes seem to get rewritten as double quotes. And since
the TOML template just prints the value as-is without any escaping, it
ends up producing an invalid TOML file.

I do wonder whether a better fix here would be to abdicate templates and
use a proper TOML serializer from an in-memory data structure, but this
fix is probably good enough for now. Namely, we use triply quoted TOML
strings. The only way this can go wrong is if the requirement contains
three single quotes in succession.

I noticed that when using a requirement like

```
c<2.0.0 ; sys_platform == 'linux'
```

then the single quotes seem to get rewritten as double quotes. And since
the TOML template just prints the value as-is without any escaping, it
ends up producing an invalid TOML file.

I do wonder whether a better fix here would be to abdicate templates and
use a proper TOML serializer from an in-memory data structure, but this
fix is probably good enough for now. Namely, we use triply quoted TOML
strings. The only way this can go wrong is if the requirement contains
three single quotes in succession.
@BurntSushi
Copy link
Member Author

It took me a fair bit of time to figure out that poetry run pytest --snapshot-update was what I needed to run to update the snapshots. Is there a place in the docs where it would make sense to document this command?

(This might have just been ecosystem ignorance on my part. What actually took me a bit to figure out was that syrupy was what was doing the snapshotting. This was surprisingly difficult to difficult to discover because import syrupy never occurs anywhere.)

@BurntSushi
Copy link
Member Author

BurntSushi commented Apr 15, 2024

It also seems to have updated a snapshot on my local system in a way that differs from CI. In particular, I had to back this snapshot update out (test_index.ambr):

@@ -1,12 +1,11 @@
 # serializer version: 1
 # name: test_index_down
   dict({
-    'exit_code': 0,
+    'exit_code': 1,
     'stderr': '''
-      Stopping server with pid [PID]...
-      Stopped server!
+      Server looks shutdown already.
   
     ''',
     'stdout': '',
   })
 # ---

@BurntSushi BurntSushi force-pushed the ag/fix-dependency-quoting branch from 31c43fb to 42f7df4 Compare April 15, 2024 14:14
@BurntSushi
Copy link
Member Author

I've added a very lightweight CONTRIBUTING.md file to document some of my learnings here.

@BurntSushi BurntSushi merged commit 0254dd4 into astral-sh:main Apr 15, 2024
5 checks passed
@BurntSushi BurntSushi deleted the ag/fix-dependency-quoting branch April 15, 2024 14:43
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 this pull request may close these issues.

2 participants