Skip to content

Conversation

@liam-mackie
Copy link
Contributor

Currently, when you use the replacement syntax {} in the startup command, it isn't replaced. Since I was hoping to use this as an argument that can't be passed with a space delimiting it, I needed to introduce a separate replacer which uses the aho-corasick algorithm to replace instances of the string. I included tests for the replacer and another test to assert the existing behaviour for preview commands.

This PR was made specifically to introduce the least change, but my preference (if you're happy with it) is to instead use the new Replacer to complete replacements in PrepareCmd before splitting and substituting the home directory. This way we have a consistent replacement mechanism. I'm also open to the replacement mechanism being used for home directory substitution as well, with PrepareCmd only being used to split into a string array. This would change behaviour, however, which I wasn't 100% on!

@joshmedeski
Copy link
Owner

Thanks for this work, I think overall it makes sense.

Can you explain why you picked aho-corasick over the built-in strings.NewReplacer from the standard library.

I'm sure it will be more performant, I'm just wondering to what extend is it "better" to introduce a new dependency.

@joshmedeski joshmedeski self-requested a review June 2, 2025 22:16
@liam-mackie
Copy link
Contributor Author

Thanks for this work, I think overall it makes sense.

Can you explain why you picked aho-corasick over the built-in strings.NewReplacer from the standard library.

I'm sure it will be more performant, I'm just wondering to what extend is it "better" to introduce a new dependency.

I mostly chose aho-corasick over strings due to my familiarity with it. The other reason is the structure of the replacements originally in the code - I assumed that there was a future direction to potentially have multiple replacements, given that we provide a map to the replacement functions. Aho-corasick is much faster when dealing with multiple replacements in a given string, and we can tweak the match functionality slightly easier too. The dependency is small and well-used, but I'm not sold on using it over the stdlib strings.Replacer if there's no future direction to have many replacements.

Copy link
Owner

@joshmedeski joshmedeski 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 the clarification, this is really good work.

I'm open to a faster tool for string substitution, and we will using this more throughout the project.

Can you please update the startup command section in the README to describe how this feature works with a practical example?

@liam-mackie
Copy link
Contributor Author

Thanks for the clarification, this is really good work.

I'm open to a faster tool for string substitution, and we will using this more throughout the project.

Can you please update the startup command section in the README to describe how this feature works with a practical example?

My mortal enemy, documentation! Just kidding - I'll get that done now and re-request a review once it's there 😄

@liam-mackie liam-mackie requested a review from joshmedeski June 3, 2025 21:41
@liam-mackie
Copy link
Contributor Author

@joshmedeski for now, I've updated the README.md with a new section for path substitution with an example (specifically, the one that I wanted to add this feature for!) - if you feel like there should be more context, or you'd like me to instead incorporate it into the existing command, let me know :)

@joshmedeski joshmedeski merged commit 307c5e2 into joshmedeski:main Jun 4, 2025
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.

2 participants