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

fix hc-scaffold cranelib filters #48

Merged
merged 4 commits into from
Sep 23, 2024

Conversation

c12i
Copy link
Contributor

@c12i c12i commented Sep 23, 2024

This PR addresses an issue where UI elements for entry/link types and collections were not being scaffolded when using the hc-scaffold nix package from holochain/holonix.

Changes made:

  1. Updated source filtering in cranelib.BuildPackage to include handlebars templates
  2. Ensured that templates for scaffolding entry/link types and collections are properly packaged

This fix resolves the inconsistency between global cargo installation and nix shell usage of the scaffolding CLI, ensuring that all necessary templates are available regardless of the installation method.


Should be backported to main-0.3

@c12i c12i marked this pull request as draft September 23, 2024 10:18
@c12i c12i marked this pull request as ready for review September 23, 2024 10:28
@c12i c12i requested a review from jost-s September 23, 2024 10:29
Copy link
Contributor

@jost-s jost-s left a comment

Choose a reason for hiding this comment

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

Just a repeated declaration, otherwise LGTM.

flake.nix Outdated Show resolved Hide resolved
@c12i c12i requested a review from jost-s September 23, 2024 13:30
@c12i c12i merged commit 687934d into holochain:main Sep 23, 2024
17 checks passed
@ThetaSinner
Copy link
Member

I'm not convinced by this. Negative includes are an easy way to make a different kind of mistake. Why not just update the positive include to cover the necessary file types?

This also means that things like uncommitted shell scripts kept in repositories (something I do) or test happ files etc, will mess with Nix's change detection and cause unnecessary rebuilds and waste space in the store.

This is one of the reasons that the new Holonix is performing better, is that it's tried to be clean about these kinds of things.

@jost-s
Copy link
Contributor

jost-s commented Sep 23, 2024

Agreed. I was too quick to approve. @c12i Please use a positive include with just the additional files you need for scaffolding.

@c12i
Copy link
Contributor Author

c12i commented Sep 23, 2024

Thanks for the feedback @ThetaSinner, I assumed the same approach I took with the scaffold package would suffice here as well. I recognize that both implementations could benefit from improvement. I'll open a separate PR to address this.

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.

3 participants