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

refactor!: Refactor to make easy improvements #30

Merged
merged 3 commits into from
Aug 5, 2024

Conversation

taorepoara
Copy link
Contributor

Has explained in the next comment, this refactor makes this project simpler to use and to improve: #24 (comment)

I've based my work on the serde way of parsing AST into structs to make it more usable.

I've also changed the way I manage attributes to make it easier to add new ones.

Copy link
Owner

@yanganto yanganto left a comment

Choose a reason for hiding this comment

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

Could we remove attribute? and then the syntax will be simple

#[patch(attribute(derive(Deserialize, Debug), serde(rename = "foo"))]

->

#[patch(derive(Deserialize, Debug), serde(rename = "foo")]

struct-patch/src/lib.rs Outdated Show resolved Hide resolved
struct-patch-derive/src/lib.rs Outdated Show resolved Hide resolved
struct-patch-derive/src/lib.rs Show resolved Hide resolved
@taorepoara
Copy link
Contributor Author

Could we remove attribute? and then the syntax will be simple

#[patch(attribute(derive(Deserialize, Debug), serde(rename = "foo"))]

->

#[patch(derive(Deserialize, Debug), serde(rename = "foo")]

That's true it would be so much simple !

We just have to be careful to avoid the possibility between the attributes and the patch configuration attributes, but if there is no way to have conflict I can do that.

I also thought about this syntax (not sure if it's possible).

What do you think?

#[patch(
  #[derive(Deserialize, Debug)],
  #[serde(rename = "foo")]
)]

@yanganto
Copy link
Owner

yanganto commented Aug 2, 2024

#[patch(
  #[derive(Deserialize, Debug)]
  #[serde(rename = "foo")]
)]

It is good and without ,, just parse everything between #[ and ] as inputs.
Such that if the developer forgot to put ,, it will not mater.

@taorepoara
Copy link
Contributor Author

#[patch(
  #[derive(Deserialize, Debug)]
  #[serde(rename = "foo")]
)]

It is good and without ,, just parse everything between #[ and ] as inputs. Such that if the developer forgot to put ,, it will not mater.

Yes ok, I'll try that. This could allow code injection but it's his own code and I'll see how the formatter manage that

@taorepoara
Copy link
Contributor Author

I could not implement a good solution for the attribute since the parse_nested_meta try to parse the sub attributes names to Iden which fails on # character.

This could be managed with more low level parsing but could not made it this morning.

I can look at that but only in a few days. I let the attribute for now.

@yanganto
Copy link
Owner

yanganto commented Aug 5, 2024

Hi @taorepoara, it looks good to me.
Is there still anything you want to refactor in the PR?

@taorepoara
Copy link
Contributor Author

Hi @taorepoara, it looks good to me. Is there still anything you want to refactor in the PR?

No, that all for this one. I'm working on another PR for other features and I'll try to implement the #[patch(#[my-attribute])] syntax later in another PR.

@yanganto yanganto merged commit 6015650 into yanganto:main Aug 5, 2024
3 checks passed
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