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

chore: convert json scenarios to toml format #211

Merged
merged 1 commit into from
Oct 4, 2024

Conversation

samypr100
Copy link
Contributor

@samypr100 samypr100 commented Aug 7, 2024

Closes #177 , #18

  • toml files where automatically generated the from the json files using a basic script
    • Used the filename as the folder name and the scenario name as the filename
  • Prettier on the generated ones for consistency

@samypr100
Copy link
Contributor Author

samypr100 commented Aug 7, 2024

Here's the basic script I used, so we can regen if needed. For prettier I used the prettier-plugin-toml as a separate follow up command.

json_scenarios_to_toml.py

import json
import re
from pathlib import Path

import toml

scenarios_path = Path(__file__).parent / "scenarios"

for path in scenarios_path.glob("*.json"):
    with path.open("r") as f:
        json_scenarios = json.load(f)

    for json_scenario in json_scenarios:
        # Use the name key as the filename
        file_path = (
            path.parent
            / re.sub(r"[\s-]+", "_", path.stem.lower())
            / f"{json_scenario['name']}.toml"
        )
        file_path.parent.mkdir(parents=True, exist_ok=True)

        toml_data = {
            "name": json_scenario["name"],
            "description": json_scenario["description"],
            "root": json_scenario["root"],
            "packages": json_scenario["packages"],
            "expected": json_scenario["expected"],
        }

        if "resolver_options" in json_scenario:
            toml_data |= {"resolver_options": json_scenario["resolver_options"] }

        if "environment" in json_scenario:
            toml_data |= {"environment": json_scenario["environment"] }

        with file_path.open("w", encoding="utf-8") as toml_file:
            toml.dump(toml_data, toml_file)
            print(f"Created: {file_path}")

@samypr100 samypr100 marked this pull request as ready for review August 7, 2024 01:17
@notatallshaw
Copy link

These are so much nicer to read.

satisfiable = false
explanation = "Only the `2.x` versions of `a` are available since `a==1.0.0` and `a==3.0.0` require incompatible versions of `b`, but all available versions of `c` exclude that range of `a` so resolution fails."

[packages.a.versions."1.0.0"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use nesting for package versions? Is that easier to read?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pre-existing fork/*.toml seem to also use this syntax, we can attempt to change it across though.

e.g. fork/filter-sibling-dependencies.toml

[packages.a.versions."4.3.0"]
[packages.a.versions."4.4.0"]
[packages.b.versions."1.0.0"]
requires = ["d==1.0.0"]
[packages.c.versions."1.0.0"]
requires = ["d==2.0.0"]
[packages.d.versions."1.0.0"]
[packages.d.versions."2.0.0"]

@zanieb
Copy link
Member

zanieb commented Aug 7, 2024

Exciting! Thanks!

@konstin
Copy link
Member

konstin commented Aug 7, 2024

Separately from this PR, do we want to add prettier to this repo?

@zanieb
Copy link
Member

zanieb commented Oct 1, 2024

I think @BurntSushi and @konstin will need to own deciding what's necessary to get this merged — I don't have strong feelings about the scenario format right now and they've been writing most of the recent scenarios.

@samypr100
Copy link
Contributor Author

samypr100 commented Oct 1, 2024

I think @BurntSushi and @konstin will need to own deciding what's necessary to get this merged — I don't have strong feelings about the scenario format right now and they've been writing most of the recent scenarios.

👍 Should I rebase? Done

Copy link
Member

@konstin konstin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this and want to merge it

@konstin konstin requested a review from BurntSushi October 2, 2024 09:20
Copy link
Member

@BurntSushi BurntSushi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yes please! I found using TOML to write scenarios border-line pleasant.

@zanieb zanieb merged commit 44c1016 into astral-sh:main Oct 4, 2024
6 checks passed
@samypr100 samypr100 deleted the json-to-toml branch October 7, 2024 12:39
@zanieb
Copy link
Member

zanieb commented Nov 4, 2024

As discovered by @charliermarsh, it looks like this conversion dropped some information

e.g.

"resolver_options": {
"python": "3.11"
},

is not present in the toml version

@notatallshaw
Copy link

notatallshaw commented Nov 4, 2024

This probably would have been a great use case to do a round trip on the formats to prove no information loss, hindsight is 20-20 and all that.

@samypr100
Copy link
Contributor Author

Apologies, not sure how I missed those. I opened #226 to port those missing entries.

zanieb pushed a commit that referenced this pull request Nov 4, 2024
#211 (comment)

Adds missing `resolver_options` and `environment` lost from json to toml
conversion
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.

Convert scenarios to TOML
5 participants