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

Fix DurationParser allowing invalid input #772

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

Conversation

emilyy-dev
Copy link
Contributor

DurationParser using that regex and Matcher#find() only finds the pattern in the given input string, while the pattern is right for individual segments denoting value-unit sequences, it does not prevent the input to have non-matching content.
This patch fixes that by manually parsing the input.

Additionally it conforms to the recommended way of parsing an argument: peek input, pop input on successful parsing.

Copy link

github-actions bot commented Sep 11, 2024

Test Results

 88 files  ±0   88 suites  ±0   12s ⏱️ -1s
434 tests +5  434 ✅ +5  0 💤 ±0  0 ❌ ±0 
479 runs  +5  479 ✅ +5  0 💤 ±0  0 ❌ ±0 

Results for commit d3911ac. ± Comparison against base commit 3c83f9a.

This pull request removes 1 and adds 6 tests. Note that renamed tests count towards both.
org.incendo.cloud.parser.standard.DurationParserTest ‑ invalid_format_failing()
org.incendo.cloud.parser.standard.DurationParserTest ‑ invalid_format_garbage()
org.incendo.cloud.parser.standard.DurationParserTest ‑ invalid_format_invalid_unit()
org.incendo.cloud.parser.standard.DurationParserTest ‑ invalid_format_leading_garbage()
org.incendo.cloud.parser.standard.DurationParserTest ‑ invalid_format_no_time_unit()
org.incendo.cloud.parser.standard.DurationParserTest ‑ invalid_format_no_time_value()
org.incendo.cloud.parser.standard.DurationParserTest ‑ invalid_format_trailing_garbage()

♻️ This comment has been updated with latest results.

@jpenilla
Copy link
Member

Additionally it conforms to the recommended way of parsing an argument: peek input, pop input on successful parsing.

This isn't the case in 2.x, the tree will restore the cursor on a failed parse. You only need to do this if your parsing logic calls for it.

@emilyy-dev
Copy link
Contributor Author

Additionally it conforms to the recommended way of parsing an argument: peek input, pop input on successful parsing.

This isn't the case in 2.x, the tree will restore the cursor on a failed parse. You only need to do this if your parsing logic calls for it.

In that case the docs need updating 👍🏻 I'll revert that change here for now

jpenilla
jpenilla previously approved these changes Sep 11, 2024
@Citymonstret
Copy link
Member

Additionally it conforms to the recommended way of parsing an argument: peek input, pop input on successful parsing.

This isn't the case in 2.x, the tree will restore the cursor on a failed parse. You only need to do this if your parsing logic calls for it.

In that case the docs need updating 👍🏻 I'll revert that change here for now

Yes, the docs should be updated. When we switched over to the CommandInput there wasn't exactly a plan to no longer require peeking. That was introduced later on to increase the comfort of using the new system. I haven't done a thorough sweep of the documentation to reflect this (although it is at the very least pointed out in the external docs now)

Citymonstret
Citymonstret previously approved these changes Sep 12, 2024
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.

3 participants