Skip to content
This repository has been archived by the owner on Jan 11, 2022. It is now read-only.

Sourcegen rewrite #46

Merged
merged 12 commits into from May 9, 2021
Merged

Sourcegen rewrite #46

merged 12 commits into from May 9, 2021

Conversation

maksimil
Copy link
Member

@maksimil maksimil commented May 3, 2021

With this PR I also want to open a conversation about some topics:

  • Is our goal to replace where possible macro calls with sourcegen?
  • I think mixin behavior is good, but traits are useful too, because of generalization. If we merge Templates for node initialization #41 we would need SandboxMemberBehavior for build and buildw methods. Or should we add them as mixins too? I mean, we can almost abandon trait if we refuse to use dyn trait objects, which might actually be a valid desicion, since we have moved to enum.

@maksimil
Copy link
Member Author

maksimil commented May 3, 2021

Also: what did you mean by checklists here. Did you mean milestones or github checklists in issues?

@philip-peterson
Copy link
Collaborator

Also: what did you mean by checklists here. Did you mean milestones or github checklists in issues?

I just mean this: I am not a fan of when projects require a checklist to be completed for every PR, because it adds a lot of friction to creating said PR. As such, there's a strict "no checklists required" policy for when PRs are opened in rdom.

Is our goal to replace where possible macro calls with sourcegen?

Unless it's a trivial or one-off macro call, yes. Generally let's try to use sourcegen to generate anything that is reasonably complex.

should we add them as mixins too?

This is a good question. For something that stands on its own like SandboxMemberBehavior, I don't see any reason we can't keep that as a trait. For something like NodeBehavior and ElementBehavior, there was a specific reason we wanted them to not be traits, which is that there's a cost, both in API design (random Result<..., SandboxDroppedError>), and also a small performance hit in following the pointers. It would be interesting to go in the direction of removing the traits for ElementBehavior and NodeBehavior and instead just having a set of methods for node, and a superset of those methods for element.

@maksimil maksimil added this to the Sourcegen rewrite milestone May 3, 2021
@maksimil maksimil marked this pull request as draft May 5, 2021 11:50
@maksimil maksimil changed the title Some sourcegen fixes and improvements Sourcegen rewrite May 5, 2021
@philip-peterson
Copy link
Collaborator

philip-peterson commented May 5, 2021

I would strongly recommend trying to get things working using sourcegen first. It's surely not a perfect tool, but it does have support behind it and is proven to work. Ideally we would spend our time working on the DOM implementation as opposed to tools surrounding it, otherwise we'll spend a bunch of time chasing rabbit holes and won't be able to accomplish the central goal... That said, you're obviously free to work on what you want, I'm just saying it will probably be [a lot 🙂] more productive to work on one thing at a time.

@maksimil
Copy link
Member Author

maksimil commented May 5, 2021

Agreed

@philip-peterson
Copy link
Collaborator

For further context, I have also thought of creating such a tool before. I ended up being woefully disappointed by the state of parsing and stringifying logic in Rust. Syn is simply not where it should be, to support projects like this; I'm pretty sure that's the reason sourcegen has to invoke cargo fmt. Just something to consider as well

@maksimil maksimil marked this pull request as ready for review May 5, 2021 15:56
@maksimil maksimil merged commit 92e5a69 into master May 9, 2021
@maksimil maksimil deleted the maksimil/small-sourcgen-fixes branch May 10, 2021 00:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants