Skip to content

Conversation

@auriium
Copy link
Owner

@auriium auriium commented Mar 29, 2021

No description provided.

@auriium
Copy link
Owner Author

auriium commented Apr 22, 2021

@A248 I've set up a large update that does quite a bit (not just adapters but they're a part of this) so go ahead and look for bugs, anything that could be optimized, any strange decisions, etc etc)

/**
* TODO: remove or fix this: i'd preferably like the internal ""initial"" deque to be sealed and immutable
* It would be nice to enforce immutability on reducablePath except the fact is the reducable path exists
* to be immutable and edited, so maybe not? Please advise.
Copy link
Contributor

Choose a reason for hiding this comment

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

A Deque is intended to be mutated and manipulated. It doesn't make much sense to have an immutable one. For immutable or read-only access I'd suggest a List.

Copy link
Owner Author

Choose a reason for hiding this comment

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

The deques will remain mutable then

<dependency>
<groupId>org.slf4j</groupId>
<artifactId>slf4j-api</artifactId>
<version>2.0.0-alpha1</version>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<version>2.0.0-alpha1</version>
<version>1.7.30</version>

2.0.0 is a bit of a tough requirement. Libraries usually allow more flexibility in transitive dependency choice. slf4j 1.7 is more widely implemented by logging frameworks than 2.0.0

Copy link
Owner Author

Choose a reason for hiding this comment

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

I think i moved this over to the test-module (but given your latest comment it will be moved back)

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yeah, this is a test only dependency so i see no point in using an older version

*
*/

package me.aurium.branch;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see why core-tests is a separate module. Usually, same-package unit tests are placed in the same module under src/test/java.

If you do want to use a separate module, you should avoid split packages across modules.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Good idea

Copy link
Owner Author

Choose a reason for hiding this comment

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

The reason for core-tests being a separate module is core-tests specific code that is potentially reusable? (string-manager)


@Override
public String easyName() {
return "none"; //TODO perhaps throw? This shouldn't happen.
Copy link
Contributor

Choose a reason for hiding this comment

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

throw if anything which shouldn't happen happens

Copy link
Owner Author

Choose a reason for hiding this comment

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

the EmptyPermission and EmptyBlock both need reworking on logic

package me.aurium.branch.nodes.builders;

/**
* yet another marker interface
Copy link
Contributor

Choose a reason for hiding this comment

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

You use so many marker interfaces, but it is not clear to me why

Copy link
Owner Author

Choose a reason for hiding this comment

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

Ensure that correctly typed classes are used where they are desired - an AloneNode is preferred for a no-arguments over just general CommandNode

*
*/

package me.aurium.branch.spigot.message;
Copy link
Contributor

Choose a reason for hiding this comment

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

What purpose do AdventureMessage and FancyTextMessage serve?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Unimplemented

Copy link
Owner Author

Choose a reason for hiding this comment

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

FancyTextMessage will allow for unified pretty formatting (e.g. all commands being red for name, grey for arguments, and a prefix of ""[KitPVP] /lah""

Copy link
Owner Author

Choose a reason for hiding this comment

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

Implementation will arrive soon!

@auriium
Copy link
Owner Author

auriium commented Apr 27, 2021

Removed annotation stuff, probably safe to merge. They will be readded as a separate module eventually.

@auriium auriium merged commit e3e5c4d into master Apr 27, 2021
@auriium auriium deleted the adapters branch May 7, 2021 21:58
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