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 support for multiple illegal characters in heading #33

Merged
merged 3 commits into from Nov 21, 2021

Conversation

scwunch
Copy link
Contributor

@scwunch scwunch commented Nov 7, 2021

The sanitizeHeading() function was using replace() with a list of strings, but that only replaces the first instance of each illegal character. This update changes the stockIllegalSymbols to a regExp object with a global flag so that all illegal characters are removed. I don't know how to do the same thing with user defined symbols, so I left that one as it is.

The `sanitizeHeading()` function was using `replace()` with a list of strings, but that only replaces the first instance of each illegal character.  This update changes the stockIllegalSymbols to a regExp object with a global flag so that all illegal characters are removed.  I don't know how to do the same thing with user defined symbols, so I left that one as it is.
@scwunch
Copy link
Contributor Author

@scwunch scwunch commented Nov 7, 2021

But next time I will try to repurpose the "removePreamble" option to ignore shortDate prefix.

And the delay (line 53) may help to make default templates work (or maybe templater renaming scripts?)
@dvcrn
Copy link
Owner

@dvcrn dvcrn commented Nov 17, 2021

Apologies, my github notifications didn't work correctly and I didn't see this until now. Thanks for the PR I'll review it asap

@scwunch
Copy link
Contributor Author

@scwunch scwunch commented Nov 17, 2021

No problem, thanks for taking a look 🙂

Oh, it looks like my second commit is included in this pull request. Sorry, I didn't realize it would do that, and my second commit has errors.

Ok, I just reverted it. I believe it will work now. Sorry, I'm new to github!

The last commit had errors and did not effectively introduce anything.
@dvcrn
Copy link
Owner

@dvcrn dvcrn commented Nov 17, 2021

The code looks fine to me! I'll give it a test later and get this merged. Can you apply yarn format to make the CI happy?

(Even better is if you could add a small unit test, but we don't have unit tests setup in here yet so this might be a bit much 😅)

@scwunch
Copy link
Contributor Author

@scwunch scwunch commented Nov 18, 2021

I'm sorry, I don't know either what yarn is nor the CI 😅

I googled yarn but I don't think my technical abilities are yet at the point where I can understand it. Is that something I need to do?

@dvcrn
Copy link
Owner

@dvcrn dvcrn commented Nov 21, 2021

No worries, I'll take the PR from here!

@dvcrn dvcrn merged commit 6fc9b64 into dvcrn:master Nov 21, 2021
1 check failed
@dvcrn
Copy link
Owner

@dvcrn dvcrn commented Nov 21, 2021

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants