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

Import template #3

Merged
merged 2 commits into from
Aug 16, 2023
Merged

Import template #3

merged 2 commits into from
Aug 16, 2023

Conversation

mattdb
Copy link
Contributor

@mattdb mattdb commented Aug 7, 2023

What is the purpose of this pull request?

Adds import_template command, as discussed in #2. (Resolves #2).

What changes did you make? (overview)

  • Added import_template method to RubyBytes::Compiler, which creates a new Compiler instance at the sub-template's path and renders it.
  • The DummyRailsTest integration test was failing for me without any changes (I changed expected value from the parameterize method to robobytes rather than robo-bytes), I included that fix in a separate commit on this PR)

Is there anything you'd like reviewers to focus on?

Does creating a new Compiler seem like the best approach (this certainly seems like the simplest)?

Thor implements its inside command using a stack of destinations, pushing and popping onto/from that stack to handle different working directories without having to create new objects to contain the state change.

I'm happy with the simpler solution provided here - as far as I can see the main downside to this approach is extra object creation, but as I don't expect this to get used more than a handful of times per template, this seems like a reasonable tradeoff. Happy to reconsider if you feel differently, though.

Checklist

  • I've added tests for this change
  • I've added a Changelog entry
  • I've updated a documentation

@palkan
Copy link
Owner

palkan commented Aug 16, 2023

Does creating a new Compiler seem like the best approach (this certainly seems like the simplest)?

Yeah, it's definitely enough for the start.

Thanks!

@palkan palkan merged commit f3e5328 into palkan:master Aug 16, 2023
0 of 4 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.

Feature Request: Nested Templates
2 participants