Simplify TextProvider, Fix all @text ending with space

[literalplus]

This PR simplifies TextProvider to be a lot clearer and also avoid swallowing an unnecessary exception.

Furthermore, it changes its behaviour so that created text does not always have a space at the end. Usually, people don't mean to add an unnecessary space at the end of their text argument if they don't put more text.

The motivation for this commit is that I am currently experiencing the space problem in my usage of this library.

Thanks!

[wizjany]

isn't the point of swallowing that exception to allow for an empty string?

[literalplus]

Wouldn't work: there's a break in the catch statement, causing first never being set to false.

Also, what's the use case?

-- Above message was sent from my mobile phone. There may and will be all kinds of errors included.

On 6 Aug 2016 22:14, "Wizjany" notifications@github.com wrote:

isn't the point of swallowing that exception to allow for an empty string?

[wizjany]

right, so it breaks the while loop and the stringbuilder will have no elements, making the toString return "", which is a completely valid arg (assuming there's no non-empty validators)

use case would be allowing an empty text arg. you could probably also use an @optional @text, but that allows it to be null afaik, meaning you need to check if it was specified or not.

[literalplus]

It breaks the while loop, first is true, so the condition on the if statement passes, and a MissingArgumentException is thrown.

-- Above message was sent from my mobile phone. There may and will be all kinds of errors included.

On 7 Aug 2016 02:32, "Wizjany" notifications@github.com wrote:

right, so it breaks the while loop and the stringbuilder will have no elements, making the toString return "", which is a completely valid arg (assuming there's no non-empty validators https://github.com/sk89q/Intake/blob/master/intake/src/main/java/com/sk89q/intake/parametric/provider/StringProvider.java#L72 )

use case would be allowing an empty text arg. you could probably also use an @optional https://github.com/optional @text https://github.com/text, but that allows it to be null afaik, meaning you need to check if it was specified or not.

[wizjany]

Ah, missed that check. Yes, both of the first conditionals would need to be changed, however i think there should still be a mechanism for allowing an empty string.

[literalplus]

Having scanned these few lines on code so often in the past day, I doubt there's a mechanism if the next() and hasNext() methods do what I think they do.

I'll check out the whole project and write a unit test to see if any of the implementations allow empty strings.

-- Above message was sent from my mobile phone. There may and will be all kinds of errors included.

On 7 Aug 2016 03:45, "Wizjany" notifications@github.com wrote:

Ah, missed that check. Yes, both of the first conditionals would need to be changed, however i think there should still be a mechanism for allowing an empty string.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/sk89q/Intake/pull/25#issuecomment-238058293, or mute the thread https://github.com/notifications/unsubscribe-auth/AAhY_osnhv90qY1uF6ht_K1ajCTzQkqQks5qdTjTgaJpZM4JeUNx .

[literalplus]

Okay, actually, you were right. Both implementations do allow to specify empty text. However, this only works by having a trailing space at the end of the command line, which I doubt is something the average user would think of.

Anyways, I provided unit tests for the behaviour of the class. They fail for the previous version because that one always ends the input with a space no matter what. From my point of view, that is clearly incorrect behaviour. As a developer, I would assume that if I pass some args with text, I would get with text instead of with text (note the trailing space). I cannot imagine a use case where you'd want that trailing space even if the user didn't even specify it.

The reason both implementations allow for empty strings (somewhat) is the way Java's String#split(...) method works: If a trailing space is found, an empty string is appended to the array. Provided tests verify that my changes do not break the possibility to provide an empty string, and that they in fact fix what I believe is incorrect behaviour.

Update: Here are the test results for old and new implementations, for reference.