Skip to content

Fix the use of '%' in ParseTests #481

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

Closed
wants to merge 1 commit into from

Conversation

McNeight
Copy link
Contributor

No description provided.

@angularsen
Copy link
Owner

Hi, thanks for the PR. Could you please refresh my memory on what this test actually tests and why you wanted to test for %% instead of %? I'm honestly puzzled by the purpose of this unit test, as it seems we want to allow parsing say 10 m!s as 10 meters. I think it was part of supporting parsing things like 1' 2" as feet/inches, but I don't think it makes much sense for Length and most other quantities.

This issue explains how the parsing is currently way too lenient and needs to be tightened a bit.
#343

This comment explains how we want to fix this, but it is a breaking change and can only be added in a major version bump.
#180 (comment)

@angularsen
Copy link
Owner

@McNeight Just a follow up, I'm waiting for your feedback on how to proceed here.

@angularsen angularsen self-assigned this Oct 1, 2018
Copy link
Owner

@angularsen angularsen left a comment

Choose a reason for hiding this comment

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

Need some feedback to continue.

@McNeight
Copy link
Contributor Author

McNeight commented Oct 5, 2018

I apologize for the delay in responding. As part of a regression test to make sure my changes didn't break anything, I ran the existing test cases and this popped up as a failed test for me.

I originally believed that the failure was due to "%" being treated as a special token within the string that needed to be escaped as "%%". I'm now unable to reproduce the original failure with a single "%", so I'm not sure what happened.

I'll take a look at the other issues you listed and dig into it a bit more, but this might be a non-issue.

@angularsen
Copy link
Owner

angularsen commented Oct 10, 2018

Just a heads up that #487 will re-implement the parse methods to be much more strict and only allow the format Length.Parse("1 m") and possibly "1m".
I propose we close this issue as it will be no longer relevant after that change is made. Please reopen the issue if there is more to discuss here.

@angularsen angularsen closed this Oct 10, 2018
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