Skip to content

MapArgument feature expansion #455

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.

Sign up for GitHub

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

Jump to bottom
Merged
willkroboth merged 30 commits into from
Jun 9, 2023
Merged

MapArgument feature expansion #455

willkroboth merged 30 commits into from
Jun 9, 2023

Conversation

@willkroboth
Copy link
Collaborator

@willkroboth willkroboth commented Jun 5, 2023 •
edited
Loading

The focus of this PR is to expand the features of the MapArgument. These include

  • Optional quoting for keys and values
  • No restriction on the characters that are allowed in a key (previously only letters, digits, and underscore)
  • String instead of char delimiters (so long delimiters like -> are possible) Nevermind, not doing that
  • Allow defining String separators between key-value pairs (instead of just space)
  • Suggestion handling for escape characters

This also generally recodes the internals of the MapArgument to use Brigadier's StringReader and merge logic shared between key and value processing. ArgumentMapTests had many (perhaps too many :P) tests added with the goal of 100% Jacoco coverage.

DerEchtePilz
DerEchtePilz requested changes Jun 5, 2023
View reviewed changes
Copy link
Member

@DerEchtePilz DerEchtePilz left a comment

Choose a reason for hiding this comment

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

So, this were just points about the delimiter. To be able to submit another review (if necessary) I need to understand this first but that's on me :D

@willkroboth willkroboth requested a review from DerEchtePilz June 5, 2023 22:48
DerEchtePilz
DerEchtePilz reviewed Jun 6, 2023
View reviewed changes
@willkroboth willkroboth requested a review from JorelAli June 8, 2023 11:07
JorelAli
JorelAli requested changes Jun 8, 2023
View reviewed changes
willkroboth and others added 23 commits June 8, 2023 17:47
@willkroboth
Implement optional quoting for MapArgument
7d69d1c
@willkroboth
Update MapArgument tests
a1232df
@willkroboth
Tweak error message when parsed key is missing delimiter, but is also…
a2c96b1 
… invalid.
@willkroboth
Address some SonarCloud code smells for MapArgument
f05438d
@willkroboth
Throw exception when map key is missing after separator
3332866
@willkroboth
Adjust map terminator finder logic
2887cc6 
Reverse escape quotes at the start of unquoted strings
@willkroboth
Update MapArgument execution tests for new features
7b3ca51
@willkroboth
Give MapArgument tests default visibility
fb76743
@willkroboth
Extract MapArgument#doseTerminatorContinue logic
02c59f5
@willkroboth
Further extract MapArgument#doseTerminatorContinue logic
9aa6c84
@willkroboth
Fix escape characters in empty suggestions
98ebe1b
@willkroboth
Update MapArgument execution tests for new features
ab7564f
@willkroboth
Automatic formatting cleanup
452d9d9
@willkroboth
Move StringParser to commandapi-core
72fd6e8
@willkroboth
Add javadocs to StringParser
35b1286
@willkroboth
Update argument_map.md documentation
2cf5083
@willkroboth
Add note for deprecated char delimiter MapArgumentBuilder constructor…
2e4bcfe 
… to upgrading.md
@JorelAli @willkroboth
Disable MD038 (no empty spaces in code) for a line.
6b4e6d2
@JorelAli @willkroboth
Fix trailing space in argument_map.md
c001ac7
@willkroboth
Throw exception when delimiter/separator are empty Strings
72c7cd9 
This case originally threw an unhelpful IndexOutOfBounds on MapArgument line 453
@willkroboth
Only allow constructing the MapArgument with char delimiters
0f22be8
@willkroboth
Remove String delimiters from ArgumentMapTests
aa044ef
@willkroboth
Remove String delimiters from MapArgument documentation
744a40e
JorelAli
JorelAli approved these changes Jun 8, 2023
View reviewed changes
Copy link
Member

@JorelAli JorelAli left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Great! I'm happy with the results. You're good to merge.
We need to make sure we don't forget to add a global changelog entry for this.

@willkroboth willkroboth merged commit 17d8b3b into dev/dev Jun 9, 2023
@willkroboth willkroboth mentioned this pull request Jul 26, 2023
Merged
@JorelAli JorelAli deleted the dev/map-argument branch November 20, 2023 00:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@DerEchtePilz DerEchtePilz DerEchtePilz approved these changes

@JorelAli JorelAli JorelAli approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@willkroboth @JorelAli @DerEchtePilz