Initial review and release 0.1.0

[sbernauer]

Whoever picks this up should review the thing we/I did came up with.
For me personally it's much more important to review the idea and concept rather than "only" the actual code lines.

Once this is done we can draw a release

[NickLarsenNZ]

I can take a look. I am happy with the concept.

[NickLarsenNZ]

I'm happy that it is test driven (and with rstest and files).

Some general comments:

• I am a little surprised by the env: prefix, but I don't mind it, and I assume it helps with better matching of things we want to change rather than dealing with literals that are supposed to go unchanged.
• I think dont_escape should change to escape.
• module structure: I think the template command stuff should all be in a template/ directory. but that can come later if/when other commands are added.
• tests: There seem to be duplicate test resource files, eg: something.properties and something.properties.expected). The former seems unused, and should be deleted.
• tests: There could be negative tests, eg: make sure not to replace stuff that shouldn't be, eg: $${env:BLAH} or \${env:BLAH} (whatever style of escaping is chosen).
• If you want github syntax highlighting for things like properties.expected, you can configure linguist in a few ways. Or just flipping the extensions (if you don't care about file ordering so much).
• Before we release, I think we should have CI setup to run tests/lints/usual stuff.

In summary: Happy with it 🚀

[sbernauer]

Many thanks for your review!

I am a little surprised by the env: prefix, but I don't mind it, and I assume it helps with better matching of things we want to change rather than dealing with literals that are supposed to go unchanged.

Yes, exactly!

I think dont_escape should change to escape.

Do we agree that escaping should be opt out? How would the cli look like than? I can only think of template --dont-escape or --escape false, would you prefer the latter?

module structure: I think the template command stuff should all be in a template/ directory. but that can come later if/when other commands are added.

Sure, 9e700cb

tests: There seem to be duplicate test resource files, eg: something.properties and something.properties.expected). The former seems unused, and should be deleted.

The flow is: Tests copies foo.properties.in -> foo.properties. config-uils in-place edits foo.properties, so we can not use this as starting point. Also the file ending .properties is needed for config-uils file type detection to work. But I improve the .gitignore in 527079c.

tests: There could be negative tests, eg: make sure not to replace stuff that shouldn't be, eg: $${env:BLAH} or ${env:BLAH} (whatever style of escaping is chosen).

The thing is that we currently don't support escaping our sequences, we have #2 for that. But you are right, we could also test things that don't work (e.g missing closing bracket - which is was to lazy to test right now)

If you want github syntax highlighting for things like properties.expected, you can configure linguist in a few ways. Or just flipping the extensions (if you don't care about file ordering so much).

Honestly I personally don't care enough, but I'm happy to collaborate on this! (renaming is sadly not possible because in-place edits, see above)

Before we release, I think we should have CI setup to run tests/lints/usual stuff.

That makes sense. On the other hand it's effort to maintain and stuff and I would hope we very rarely touch this repo. But I guess let's do at least cargo clippy and cargo test

[sbernauer]

Added some CI to #4 as well

[NickLarsenNZ]

I think dont_escape should change to escape.

Do we agree that escaping should be opt out? How would the cli look like than? I can only think of template --dont-escape or --escape false, would you prefer the latter?

tests: There seem to be duplicate test resource files, eg: something.properties and something.properties.expected). The former seems unused, and should be deleted.

The flow is: Tests copies foo.properties.in -> foo.properties. config-uils in-place edits foo.properties, so we can not use this as starting point. Also the file ending .properties is needed for config-uils file type detection to work. But I improve the .gitignore in 527079c.

Oh cool, yeah so they were generated and will no longer be in the repo.

tests: There could be negative tests, eg: make sure not to replace stuff that shouldn't be, eg: $${env:BLAH} or ${env:BLAH} (whatever style of escaping is chosen).

The thing is that we currently don't support escaping our sequences, we have #2 for that. But you are right, we could also test things that don't work (e.g missing closing bracket - which is was to lazy to test right now)

👍

If you want github syntax highlighting for things like properties.expected, you can configure linguist in a few ways. Or just flipping the extensions (if you don't care about file ordering so much).

Honestly I personally don't care enough, but I'm happy to collaborate on this! (renaming is sadly not possible because in-place edits, see above)

I do [care] :) and am happy to do this.

Before we release, I think we should have CI setup to run tests/lints/usual stuff.

That makes sense. On the other hand it's effort to maintain and stuff and I would hope we very rarely touch this repo. But I guess let's do at least cargo clippy and cargo test

@xeniape mentioned wanting to do a CI piece (I think that was for docker-images), but maybe she is also keen to do this one? (already done in #4)

[sbernauer]

Many thanks for the review!