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

Enable to use odd filenames #53

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

delphinus
Copy link
Contributor

  1. 75ce5c5 Fix errors in creating notes including $ in filenames.

    # master build
    > nb create --filename 'foo$bar$.md'
    /usr/local/bin/nb: line 1263: bar: unbound variable
  2. eabc1b1 Enable to get IDs even when filenames have [ * chars.

    > nb add --filename '[foo].md'
    > nb add --filename '*foo*.md'
    
    # master build
    > nb list
    # prints no line
    
    # this PR
    > ./nb list
    [2] *foo*
    [1] [foo]

@delphinus
Copy link
Contributor Author

For performance issue, we should use bash parameter expansion for replacing [ * in patterns, maybe.

escaped="${__basename:-}"
escaped=${escaped//\[/\\[}
escaped=${escaped//\*/\\*}
# now escaped completely

But this seems redundant for me. So I used sed to replace. Which do you prefer?

escaped=$(printf "%s" "${__basename:-}" | sed -e 's/[\[\*]/\\&/g')

@xwmx
Copy link
Owner

xwmx commented Oct 22, 2020

Thanks! I'll check it out.

Parameter expansion is usually faster since it doesn't create a subshell and call sed. I've had to replace printf and sed with bash-specific solutions in a few places like _get_title() because it's more performant. Command substitution is one of the things that appears to be slowing down list.

I do a lot of benchmarking with time and, more recently, hyperfine, so we should determine the performance impact of each approach. I've been trying to pare the list loop down and optimize _index get_id to identify and isolate the bottlenecks.

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