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

Try to generate more meaningful names in json converter #17115

Merged
merged 2 commits into from Apr 21, 2024

Conversation

leviska
Copy link
Contributor

@leviska leviska commented Apr 20, 2024

I just found out about rust-analyzer json converter, but I think it would be more convenient, if names were more useful, like using the names of the keys.

Let's look at some realistic arbitrary json:

{
    "user": {
        "address": {
            "street": "Main St",
            "house": 3
        },
        "email": "example@example.com"
    }
}

I think, new generated code is much easier to read and to edit, than the old:

// Old
struct Struct1{ house: i64, street: String }
struct Struct2{ address: Struct1, email: String }
struct Struct3{ user: Struct2 }

// New
struct Address1{ house: i64, street: String }
struct User1{ address: Address1, email: String }
struct Root1{ user: User1 }

Ideally, if we drop the numbers, I can see it being usable just as is (may be rename root)

struct Address{ house: i64, street: String }
struct User{ address: Address, email: String }
struct Root{ user: User }

Sadly, we can't just drop them, because there can be multiple fields (recursive) with the same name, and we can't just easily retroactively add numbers if the name has 2 instances due to parsing being single pass.
We could ignore the 1 and add number only if it's > 1, but I will leave this open to discussion and right now made it the simpler way

In sum, even with numbers, I think this PR still helps in readability

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 20, 2024
*count += 1;
*count
} else {
self.names.insert(name.clone(), 1);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't want to copy if name exists, so this long if

@leviska
Copy link
Contributor Author

leviska commented Apr 20, 2024

I just remembered, that the name in json can be invalid struct name... I'll try to find a way to check that I guess

@leviska
Copy link
Contributor Author

leviska commented Apr 20, 2024

Actually, the old code fails at this too, because it doesn't fix the names for struct member variables...

@Veykril
Copy link
Member

Veykril commented Apr 21, 2024

Thanks!
@bors r+

@bors
Copy link
Collaborator

bors commented Apr 21, 2024

📌 Commit 029c710 has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Apr 21, 2024

⌛ Testing commit 029c710 with merge 3077e69...

@bors
Copy link
Collaborator

bors commented Apr 21, 2024

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing 3077e69 to master...

@bors bors merged commit 3077e69 into rust-lang:master Apr 21, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants