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 standard join configurations #482

Merged

Conversation

lynxplay
Copy link
Contributor

While using join configurations is already rather simple, they tend to
clutter up already rather verbose source code. While the initial
creation of the JoinConfiguration already included a null configuration,
accessible through 'JoinConfiguration#noSeparators', other standards or
often used layouts were not added.

This commit adds three more defaults/standards to the join configuration
implementation that are exposed through static accessors in the join
configuration interface.
Specifically:

newLines:
A straight forward instance of the join configuraiton that uses the
new line component to join together components.

commas:
Another straight forward instance of the join configuration that uses
a single comma to join together components, creating a CSV like
layout.

arrayLike:
A more complex join configuration that recreates the layout used by
the java.util.Arrays#toString method.

Resolves: #466


Pretty open about suggestions for the arrayLike method name.
If others have more suggestions for often used standard styling (tho arrayLike and newLines are by far my most used ones) I'd be happy to implement them in the context of this PR as well.

@lynxplay lynxplay force-pushed the feature/standard-join-configurations branch from edd71d8 to ee77198 Compare October 24, 2021 00:45
@lynxplay lynxplay requested a review from kashike October 24, 2021 12:05
@lynxplay lynxplay force-pushed the feature/standard-join-configurations branch from ee77198 to 4e565d4 Compare October 24, 2021 14:58
@rymiel
Copy link
Member

rymiel commented Oct 24, 2021

I wonder what would be more expected; separated,by,commas or separated, by, commas. Both seem to have their usecases and it seems very difficult to concisely set them apart in a short method name

Copy link
Member

@kezz kezz left a comment

Choose a reason for hiding this comment

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

I like these changes overall. I agree with Emilia, might be worth thinking about if a space would be more expected. Not sure if I'm convinced either way tbh..

@lynxplay
Copy link
Contributor Author

Maybe providing both options is the best way to go ? E.g. instead of #commas we also create a #spacedCommas ? The existence of the method should clarify the fact that #commas only includes a comma.

@lynxplay lynxplay force-pushed the feature/standard-join-configurations branch from 4e565d4 to 8e2042e Compare October 24, 2021 17:14
@lynxplay
Copy link
Contributor Author

Added the commasSpaced method as by my initial suggestion. However I would be very happy for more feedback about the naming.

@lynxplay lynxplay requested a review from kezz October 24, 2021 20:00
@kezz
Copy link
Member

kezz commented Oct 24, 2021

Hmmm.. Maybe just the one method with a boolean parameter for a space?

@lynxplay
Copy link
Contributor Author

Hmmm.. Maybe just the one method with a boolean parameter for a space?

Yea would have been my second guess 👍 I'll stick to that then :)

Copy link
Member

@kezz kezz left a comment

Choose a reason for hiding this comment

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

Some minor nitpicks

@lynxplay lynxplay force-pushed the feature/standard-join-configurations branch from 8e2042e to 2987ef9 Compare October 24, 2021 21:46
@lynxplay
Copy link
Contributor Author

Updated the additions to a comma(boolean) style and fixed your nitpicks 🙏

Copy link
Member

@kezz kezz left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the PR :)

@lynxplay lynxplay force-pushed the feature/standard-join-configurations branch from 2987ef9 to 4368f68 Compare October 24, 2021 21:55
While using join configurations is already rather simple, they tend to
clutter up already rather verbose source code. While the initial
creation of the JoinConfiguration already included a null configuration,
accessible through 'JoinConfiguration#noSeparators', other standards or
often used layouts were not added.

This commit adds three more defaults/standards to the join configuration
implementation that are exposed through static accessors in the join
configuration interface.
Specifically:

newLines:
  A straight forward instance of the join configuraiton that uses the
  new line component to join together components.

commas:
  Another straight forward instance of the join configuration that uses
  a single comma (or potentially a comma and a space) to join together
  components.

arrayLike:
  A more complex join configuration that recreates the layout used by
  the java.util.Arrays#toString method.

Resolves: KyoriPowered#466
@lynxplay lynxplay force-pushed the feature/standard-join-configurations branch from 4368f68 to 5d4384a Compare October 24, 2021 22:01
This was referenced Feb 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add some "standard" JoinConfiguration instances
5 participants