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

Add the option to retrieve raw arguments #459

Merged
merged 32 commits into from Jul 12, 2023
Merged

Conversation

DerEchtePilz
Copy link
Collaborator

@DerEchtePilz DerEchtePilz commented Jun 8, 2023

I think this needs a little bit more work but I am still opening the PR now

ToDo's:

  • Add tests
  • Write documentation
    • new page
    • link every appearance of CommandArguments to that new page
    • Update upgrading.md
    • Update intro.md
    • Update optional_arguments.md
    • Add examples
  • Remove @Deprecated annotation from the CommandArguments#getOrDefaultUnchecked methods because the CommandArguments#getOptionalUnchecked methods require you to provide a type or cast it which is not great
  • Update JavaDocs for all of the getRaw methods. They should explain what a "raw argument" is
  • Update every example to use the node name to access arguments instead of the index

Copy link
Owner

@JorelAli JorelAli left a comment

Choose a reason for hiding this comment

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

Don't use toArray(String[]::new), use toArray(new String[0]) instead when converting from a List<String> (especially an ArrayList<String>) to a String[]

@JorelAli
Copy link
Owner

JorelAli commented Jun 8, 2023

I'm not entirely sold on the naming convention of "raw", but I don't have any better ideas.

@DerEchtePilz
Copy link
Collaborator Author

Well, I thought that if something wasn't dealt with already, it exists in its raw form...
I can totally think about the name again but in my opinion it kinda fits.

@JorelAli
Copy link
Owner

JorelAli commented Jun 8, 2023

Well, I thought that if something wasn't dealt with already, it exists in its raw form... I can totally think about the name again but in my opinion it kinda fits.

If you're happy with it, I'm happy with it. I got nothing. Just have to make sure it's clear in the documentation what we mean by a "raw" argument as not to cause any misunderstandings.

@JorelAli
Copy link
Owner

Other than writing documentation for this, is this feature-complete?

@DerEchtePilz
Copy link
Collaborator Author

Yeah, I think so.
I would also add some basic tests somewhere but I put tests on the same level as documentation writing so that will happen in one step.
I would wait to merge this (if that's what you want) because then I'll probably forget about documentation xD

@JorelAli
Copy link
Owner

I'm only willing to merge this when documentation has been written. Good shout for adding tests, I forgot about that!

@DerEchtePilz
Copy link
Collaborator Author

About the documentation, currently I am not very motivated to do this (partly because I want to add a complete new page for the CommandArguments class) so I will get to it at some point later.
This isn't urgent, right (so I can take the time I need)?

@JorelAli
Copy link
Owner

JorelAli commented Jun 11, 2023

currently I am not very motivated to do this

Fair enough, I've still not even started working on the overhauled annotation system yet. These things just happen!

This isn't urgent, right (so I can take the time I need)?

Yes, this is not an urgent feature, feel free to take all the time you need.

Copy link
Collaborator

@willkroboth willkroboth left a comment

Choose a reason for hiding this comment

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

Just some quick looks at the code. Only little tweaks that don't really have a big impact either way, so you could totally just resolve the comments and not use them if you think the current way is fine.

Sorry, I couldn't get to the documentation because I have to go now. I can just suggest looking through the GitHub Actions failures, which is complaining about the spacing between some of the lines (all lines should have 1 empty line between them).

Copy link
Owner

@JorelAli JorelAli left a comment

Choose a reason for hiding this comment

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

Documentation reviewed as requested. I've requested a couple changes, notably:

  • A bit of re-organizing of the CommandArguments page by removing the table-of-contents and the "What terms are used?" sections
  • Changing the format of "Access the inner structure directly" to match the way other pages in the documentation explain similar concepts

There's a lot of repetition with the node name and accessing by index under each section! After a long think about it, I'm going to approve that - I don't think it needs changing, but ensure that when it comes to writing examples you keep them concise - an example isn't necessary for every single method and once you've given an example of one concept you don't have to repeat it for every one (for example, you can write an example for get(String) and get(int), an example for getOrDefault(String, Object/Supplier) and getOptional(String) in the "Access arguments" section. In the "access raw arguments" section, an example of just accessing getRaw(String) would be sufficient and likewise with the "access unsafe arguments" section - an example of getUnchecked(String) would be sufficient).

An explanation at the top of "access raw arguments" is necessary to let users know what we're talking about (refactoring the explanation in "What terms are used" to this section would make a lot of sense here). Likewise with unsafe arguments.

Hopefully this review isn't too long!

docssrc/src/optional_arguments.md Show resolved Hide resolved
docssrc/src/commandarguments.md Outdated Show resolved Hide resolved
docssrc/src/commandarguments.md Outdated Show resolved Hide resolved
docssrc/src/commandarguments.md Outdated Show resolved Hide resolved
docssrc/src/commandarguments.md Outdated Show resolved Hide resolved
docssrc/src/commandarguments.md Outdated Show resolved Hide resolved
@JorelAli JorelAli self-requested a review July 5, 2023 11:31
Copy link
Owner

@JorelAli JorelAli left a comment

Choose a reason for hiding this comment

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

Other than the issue with JavaDocs for the CommandArguments record, we're ready to merge.

@DerEchtePilz DerEchtePilz merged commit 116306e into dev/dev Jul 12, 2023
2 of 3 checks passed
@JorelAli JorelAli deleted the dev/raw-argument-input branch November 20, 2023 00:23
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.

None yet

3 participants