-
Notifications
You must be signed in to change notification settings - Fork 109
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
Experimental V2 renderer #479
base: release-2.0
Are you sure you want to change the base?
Conversation
0b5e538
to
9bcc80a
Compare
f1447e9
to
8c7354e
Compare
5113df7
to
519101b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
I would consider two things that I don't see here (unless I missed it).
- Implement association as it's own thing, instead of having that be just a field. Fields are inherently different from associations, and it'll be easier to work with if that's reflected in the internal code as well. related issue
- Separate internal options from external ones, instead of having some 'blessed' or 'special' options among user-defined ones. related issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing this is temporary, but I wouldn't add a V2
namespace. Instead I'd just bump the gem version.
Much less work, and less confusing, to maintain multiple versions that way.
76e930f
to
b7a8adc
Compare
I think this is accounted for. I've actually split "assocation" into "singular" and "multiple" variants (currently with the placeholder names
This is a little tricky given the hook-based design. To ensure that extensions can be just as powerful as most built-in functionality, all built-in options (if, unless, default, root, extractor, etc) are implemented with extension hooks. But I suppose we could still store those options separately if there's a consensus around it. I did move |
bb49db7
to
0ee79d7
Compare
Signed-off-by: Jordan Hollinger <[email protected]>
Signed-off-by: Jordan Hollinger <[email protected]>
Signed-off-by: Jordan Hollinger <[email protected]>
Signed-off-by: Jordan Hollinger <[email protected]>
Signed-off-by: Jordan Hollinger <[email protected]>
Signed-off-by: Jordan Hollinger <[email protected]>
Signed-off-by: Jordan Hollinger <[email protected]>
…k hooks that aren't used Signed-off-by: Jordan Hollinger <[email protected]>
Signed-off-by: Jordan Hollinger <[email protected]>
Signed-off-by: Jordan Hollinger <[email protected]>
0ee79d7
to
630f6f1
Compare
Ah, I see now. I was looking at this PR diff, but the true diff seems to be this one:
I'm just providing feedback here, based on some thoughts I had when we implemented Blueprinter in our codebase. I understand, though I guess I also slightly disagree with the idea to implement built-in functionality using extension hooks. I think I would have taken a different path, but it's also quite subjective so take this with a grain of salt 😄 Overall, great to see the new V2 taking shape! 🎉 |
6d6ab4e
to
3c3af70
Compare
Signed-off-by: Jordan Hollinger <[email protected]>
3c3af70
to
977e832
Compare
An experimental serializer/renderer for the proposed V2 base. Rapidly evolving. I know it looks large, but most of that is tests. It's broken up into small commits so we can easily pull out pieces and PR them individually if we want. Notes:
WidgetBlueprint
instance).Performance
The perf characteristics of V1 and V2 vary greatly based on:
Note that on the very small end of things (few items with few fields) V2 is actually slower. But we're talking fractions of a ms.