-
Notifications
You must be signed in to change notification settings - Fork 230
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
Support relative path for snippet's filename attribute #322
base: main
Are you sure you want to change the base?
Conversation
@RamiAwar could you review this PR, please? |
How are you handling interaction with snippet directories? |
Directories can also be expressed in relative path format, e.g. |
validate string, raise err and other minor typos
Resolved conflicts |
Awesome, thank you! @csessh Will review this carefully this week. Thanks for waiting 🙏🏼 ✨ |
for _, dir := range cfg.General.SnippetDirs { | ||
snippetdirs = append(snippetdirs, expandPath(dir)) // note the = instead of := | ||
if !strings.HasSuffix(dir, "/") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I recall correctly, that's because it would lead to new snippet filename being incorrect.
Line 94 in adfb69a
snippetFile = config.Conf.General.SnippetDirs[0] + fmt.Sprintf("%s.toml", strings.ToLower(sanitize.BaseName(snippet.Description))) |
for example:
/var/tmp/petNewCommand.toml
instead of
/var/tmp/pet/NewCommand.toml
validate string, raise err and other minor typos
closes #320
This pull request contains changes to support both relative and absolute path types for snippet file's filename attribute.
Given the test case listed in #320:
The same case presented in the issue
Given this config:
SnippetFile
is a relative path which expands to/home/csessh/.config/test/test.toml
Current Pet version:
As expected,
~/.config/test/test.toml
expands to/home/csessh/.config/test/test.toml
Using feature branch version:
file path stays the same. The same result applies for the following test case:
Full path is rendered for every snippets.
I didn't go too deep into testing filename with the use of multiple directories as I ran into #321 problem. However, I did include the safeguard check for it in this PR too.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.