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 current version to the generated config without hard coding it in the actual template. #73

Closed
MitchellBerend opened this issue Sep 8, 2022 · 6 comments · Fixed by #78
Labels
config TOML configuration, config-related CLI options

Comments

@MitchellBerend
Copy link
Collaborator

Referencing #62

Would be cool to also say by which version of tool-sync but not sure how to do this without hardcoding the version

We can do some magic in a build.rs file that can generate the src/config/template.rs so it contains the version in the config template itself.

Im not quite sure if a build.rs is the best way to implement this since this means we have to have a template for the src/config/template.rs file itself. This sounds like a very weird approach to me but I currently don't see any other way to do this.

The build.rs file would look something like

const TEMPLATE_RS_TEMPLATE: &str = r##"
The actual template
"##


fn main() -> std::io::Result<()> {
  std::fs::write_all("src/config/template.r", TEMPLATE_RS_TEMPLATE)?;
}
@chshersh chshersh added the config TOML configuration, config-related CLI options label Sep 8, 2022
@chshersh
Copy link
Owner

chshersh commented Sep 8, 2022

Another option would be to use the CARGO_PKG_VERSION build time environment variable and the format! macro so insert the version inside the template:

It would require to either move the config from a separate variable inside the format!() macro or using a template engine library. I haven't used any but I'd love to avoid unnecessary extra dependencies 🤔

@MitchellBerend
Copy link
Collaborator Author

MitchellBerend commented Sep 8, 2022

Another option would be to use the CARGO_PKG_VERSION build time environment variable and the format! macro so insert the version inside the template:

Would this not still mean we generate the template at compile time, or am I misunderstanding this?

Edit: Did you mean using this environment variable instead of parsing the Cargo.toml? That does actually make sense if generating the template is the direction we decide to go with.


but I'd love to avoid unnecessary extra dependencies.

Agreed.

@chshersh
Copy link
Owner

chshersh commented Sep 9, 2022

Edit: Did you mean using this environment variable instead of parsing the Cargo.toml? That does actually make sense if generating the template is the direction we decide to go with.

Yes, I did mean using the environment variable. As I understand, it's passed by cargo during the build time.

I think I'd like to move towards the direction of generating the default configuration. A few things that come to my mind:

  • Automatically embedding version. As discussed in this issue.
  • Prefill all the tools supported by tool-sync. This requires converting the big match to a BTreeMap.
  • Add comments around tool tables. To make sure that the code inside the comments stays up-to-date and is parsed by tool-sync itself.

@MitchellBerend
Copy link
Collaborator Author

MitchellBerend commented Sep 9, 2022

Prefill all the tools supported by tool-sync. This requires converting the big match to a BTreeMap.

Should we also put the actual definitions of these in a config.toml as well so the tools can be edited without changing the code? This way others can also just fork and edit the config and build from source themselves much easier.

Edit: formatting

@chshersh
Copy link
Owner

chshersh commented Sep 9, 2022

Should we also put the actual definitions of these in a config.toml as well so the tools can be edited without changing the code?

Interesting question 🤔 My first thought is to not put the details inside the config about each hardcoded tool to have a smaller default config and show that people don't need to uncomment all these details. Having a single example of a fully-detailed tool (not hardcoded by tool-sync) would be enough as people can copy only relevant fields.

When you want to fork a project and get binaries from your own fork, you only need to change the owner field which is not a lot.

People can always delete redundant comments but my gut feeling is that people will rarely have customized forks and more often they would just want a list of tools to download. So I'm proposing to optimize for the most common use case.

Of course, time will show and we can always change the defaults later if we realise this is not the case 🙂

@MitchellBerend
Copy link
Collaborator Author

So I'm proposing to optimize for the most common use case.

Ye good point, I thought this might have been easier to maintain as well since it would just be a change in the config and rebuild.
But that would also incur more work to maintain the feature as well, which might not even be used.

chshersh pushed a commit that referenced this issue Sep 10, 2022
This pr adds a `.pre-commit-config.yaml` to the repo so others can
install a
[pre-commit](https://pre-commit.com/).

The options added are in the suggested list of the pre-commit project.
chshersh added a commit that referenced this issue Sep 15, 2022
…ersion (#78)

Resolves #73
This adds a build script that generates the `src/config/template.rs`
file. It is
done this way because there is no access to version information after
compile
time. It uses the `CARGO_PKG_VERSION` environment variable made
available for
cargo after running the command `cargo build`.

### Additional tasks

tasks following #73

---

- [x] Automatically embedding version. As discussed in this issue.
- [x] Prefill all the tools supported by tool-sync. This requires
converting the
    big match to a BTreeMap.
- [x] Add comments around tool tables. To make sure that the code inside
the
    comments stays up-to-date and is parsed by tool-sync itself.

General tasks

---

- [ ] Documentation for changes provided/changed
- [x] Tests added

Co-authored-by: Dmitrii Kovanikov <kovanikov@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
config TOML configuration, config-related CLI options
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants