Skip to content

Conversation

mariuskoeppen
Copy link

This PR adresses the following issues:

  • fix: cargo-generate command in README.md was pointing to a non-existent repo
  • removed opinionated VSCode settings in favor of more generic rust-analyzer.toml, which should work with more IDEs
  • removed Cargo.lock, as this file should not be part of a library
  • include some default [profile.dev] optimizations
  • updated Trunk.toml to reflect updated trunk settings options
  • fix: 404 page was not included properly (resolves fix fallback/404 view for router #13)

Copy link
Contributor

@gbj gbj left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time to make a PR! I'm fine with most of this but there are some things I'd prefer to see reverted to the current state of things (see comments)

Cargo.lock Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not a library, it's an application starter template. Including the lockfile is helpful because it ensures that the template will always build correctly, even if a dependency breaks in a later version. Note that the Cargo team has recommended committing Cargo.lock by default, even for libraries, for a couple years.

Copy link
Contributor

Choose a reason for hiding this comment

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

These changes seem a bit opinionated for a template. I would leave them as is and let people tweak the settings if they want to.

Copy link
Contributor

Choose a reason for hiding this comment

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

(See same comments on the nightly file)

Trunk.toml Outdated
@@ -14,17 +20,19 @@ filehash = true
# Whether to inject scripts (and module preloads) into the finalized output.
inject_scripts = true
# Run without network access
# offline = false
offline = false
Copy link
Contributor

Choose a reason for hiding this comment

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

The changes to add additional features to this file also (inadvertently?) uncommented a bunch of settings that were previously commented out.

@@ -1,16 +1,16 @@
[template]
ignore = ["Cargo.lock", "switch_nightly_stable.rhai"]
ignore = ["switch_nightly_stable.rhai"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be restored with lockfile

@mariuskoeppen
Copy link
Author

You are right, I put the Cargo.lock file back in. Also, I commented out the lines in Trunk.toml.

For the dev profile optimizations in Cargo.toml, I commented them out, but I can fully remove them if you like.

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