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

Add lacci gem #309

Merged
merged 14 commits into from
Jul 14, 2023
Merged

Add lacci gem #309

merged 14 commits into from
Jul 14, 2023

Conversation

noahgibbs
Copy link
Collaborator

@noahgibbs noahgibbs commented Jul 11, 2023

This builds on PR #307.

Description

Add a gem called lacci (already reserved on rubygems.org) to act as the portable Shoes DSL part of Scarpe. Make scarpe depend on it, but otherwise try to give it no -- or minimal -- dependencies. Move all the Shoes widgets into Lacci, and rename them to clearly be in the Shoes namespace.

Split logging into a portable, impl-independent interface and a Scarpe-specific Logging-based implementation.

I still need to fix the ShapeHelper problem, where it's a dependency in Webview that the Lacci shapes have. I have a partially-finished PR to do that that I'll rework for the new structure in this PR.

Checklist

  • Run tests locally
  • Run linter(check for linter errors)

@noahgibbs noahgibbs force-pushed the add_lacci_gem branch 3 times, most recently from bd0fe4c to c3f63b7 Compare July 11, 2023 20:33
@noahgibbs noahgibbs marked this pull request as ready for review July 11, 2023 20:40
@noahgibbs noahgibbs mentioned this pull request Jul 13, 2023
2 tasks
Copy link
Collaborator

@Schwad Schwad left a comment

Choose a reason for hiding this comment

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

I'm going to echo back my understanding here from reading through the Discord, this code, and the description.

  1. A full extraction (and correct-naming) of the shoes-dsl component of our gem into the new lacci gem.
  2. This gem remains in our directory and we depend on it directly.
  3. This makes the separation more of a first class citizen. More importantly, it helps support the WASM work. We have one place where we extend and continue to develop the DSL support. e.g. Maybe the webview people will figure out url first when they implement in lacci and Webview, and then the WASM folks will just have to implement in WASM as it's already in lacci.
  4. Will this break @gintama91's generator code? Sorry if you've already referenced this in an issue, I'm looking at PRs first.
  5. I know I've done the only scarpe push in the last few months, but I might want to make a releasing guidance (unless we already have it) as a reminder to at least myself that we should probably release these in tandem. or maybe even make a slick script for the gem build/gem push for both gems. 😅

I'm a hyper-averse person to early abstractions; and I think we've done well not abstracting too early. If my understanding is in the right ballpark with WASM then yes abstracting to a new gem totally makes sense here.

Feel free to merge but I'll leave that on your side.


Gem::Specification.new do |spec|
spec.name = "lacci"
spec.version = Scarpe::VERSION
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like binding this to the scarpe version

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I think that's a good idea for some time yet. Eventually we're going to need to distinguish between "Scarpe, the Webview-based display library" and "Scarpe, the layer of display implementation for Lacci, especially logging and testing code." In the long run they're not the same thing. Wasm is going to push us into splitting it into more pieces long-term. All these divisions were kind of theoretical, right up until they weren't :-)

@noahgibbs
Copy link
Collaborator Author

I'm going to echo back my understanding here from reading through the Discord, this code, and the description.

Yup. This is all basically correct. Some minor notes:

  1. This makes the separation more of a first class citizen. More importantly, it helps support the WASM work. We have one place where we extend and continue to develop the DSL support. e.g. Maybe the webview people will figure out url first when they implement in lacci and Webview, and then the WASM folks will just have to implement in WASM as it's already in lacci.

Yup. But also WASM and Webview are going to share a lot of stuff that isn't in Lacci and shouldn't be -- e.g. HTML generation. Lacci is still the display-independent part, just like the Scarpe/Shoes non-display widgets have always been. But yeah, once one implementation adds it in Lacci it should just work for everybody.

  1. Will this break @gintama91's generator code? Sorry if you've already referenced this in an issue, I'm looking at PRs first.

Ooh, good call. Probably. I'll go check that.

  1. I know I've done the only scarpe push in the last few months, but I might want to make a releasing guidance (unless we already have it) as a reminder to at least myself that we should probably release these in tandem. or maybe even make a slick script for the gem build/gem push for both gems. 😅

Not a bad idea. I need to give you permissions on lacci.

@noahgibbs noahgibbs merged commit fd3d10a into main Jul 14, 2023
@noahgibbs noahgibbs deleted the add_lacci_gem branch August 1, 2023 09:37
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