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

Implement function to tie percent sign to number #201

Closed
wants to merge 3 commits into from
Closed

Implement function to tie percent sign to number #201

wants to merge 3 commits into from

Conversation

Badboy-16
Copy link

Description

Fixes #196

Adds a function to tie the percent sign (%) to the preceding numbers.

Type of PR

  • Bugfix
  • Feature implementation
  • Refactor of code (without functional changes)
  • Documentation improvements
  • Test improvements

Testing

Testing added in test_parse_common.py. (test_normalize())

@devs-mycroft devs-mycroft added the CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) label Jun 11, 2021
@ChanceNCounter
Copy link
Contributor

ChanceNCounter commented Jun 11, 2021

Sorry, I think I failed to communicate the core problem with #196.

The larger issue is that the normalizer is returning "X %.", instead of "X % .", because it doesn't generally insert a space before punctuation. Consumers looking for that percent sign in isolation miss it when it has punctuation attached to it.

The space between the number and the percent sign is apparently useful downstream, so we might have to revert that part.

@Badboy-16
Copy link
Author

It's ok @ChanceNCounter

So in that case, should the behaviour of normalizer be that it inserts a space between the percent sign and the punctuation? If so, I think I can revert my previous code and add a function to check if any percent signs preceding any punctuations, and if yes, insert a space between them.

@ChanceNCounter
Copy link
Contributor

That's the desired behavior, yes. I dunno if the other discussion is closed forever =P but that's the part that should change for now. The trouble with changing behavior such as the separate percent sign is that existing code might be looking for that output.

We can make changes like that, with an appropriate deprecation period, but it's no small thing and the discussion has to brew for a while. Apparently our predecessors put that space in there on purpose.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Normalizer mishandles "X%.", returns "X %."
3 participants