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

🐛 customization.getnames: respect protected names #334

Merged
merged 3 commits into from
Oct 12, 2022

Conversation

tdegeus
Copy link
Collaborator

@tdegeus tdegeus commented Oct 11, 2022

Fixes #327

I worked on #327 all while not noticing #331 .

Not surprisingly the solution is the same, with the difference that the current PR only breaks at spaces outside a sequence protected by "{...}" in line with the rest of the implementation of getnames. This reduces some of the worries discussed there I believe

Update: I was reading #331 and realised that I could do things better (safer). It does account for escaped brackets too. I think that this PR should be merged in favour of #331 . BTW, it seems to fix #9 as well that I happen to find in the tests . Note that there is a panic-switch: in case of unmatched brackets the split is entirely not applied. Instead there could be an error (or warning) in my opinion.

For reference, I took the implementation from my own code ( https://github.com/tdegeus/texplain/blob/21d0d50aa10a6a9a3ee437e0c374d088142da709/texplain/__init__.py#L80 ) licensed MIT

@QuantumNovice @MiWeiss

@MiWeiss MiWeiss changed the title customization.getnames: respect protected names 🐛 customization.getnames: respect protected names Oct 12, 2022
@MiWeiss
Copy link
Collaborator

MiWeiss commented Oct 12, 2022

Hi @tdegeus

Thanks a lot for this PR! I guess we're very close to merging it.

P.S. Next time you work on any issues you identify (which would be highly appreciated), please leave a corresponding remark in the issue. I'll then assign it to you, making sure that no one else works on the issue. Sorry to @QuantumNovice - I'll reply to you in your PR.

@tdegeus
Copy link
Collaborator Author

tdegeus commented Oct 12, 2022

Indeed, I'm very sorry @QuantumNovice . I started working on it even after your latests commit. So I was frustrated with myself and wanted to close my PR. However, the discussion you guys had over there made me realise that I actually had something to contribute on top of #331 in terms of escaped brackets, as that is something that I had solved in the passed.

Copy link
Collaborator

@MiWeiss MiWeiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm! thanks!

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

Successfully merging this pull request may close these issues.

getnames edge-case getnames incorrectly parsing names with braces
2 participants