Skip to content

maybe() method now accepts a builder as parameter #34

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 3 commits into from
Feb 5, 2015

Conversation

Tavio
Copy link

@Tavio Tavio commented Feb 3, 2015

I propose that all Builder methods which accept a string as parameter should have another version that accepts another Builder as parameter. This would allow users to break their expressions into smaller expressions, thus making their code more readable and maintainable. I built a test showing an example of an expression built with the oneOf() method being passed as the parameter for the maybe() method (with the current code this would not be possible, as the contents of the oneOf() expression would all be escaped with ).

If you guys think this is a good idea, I will open another PR implementing the same idea for all other methods of the Builder class.

@dummy-lanwen-bot
Copy link

Can one of the admins verify this patch?

@lanwen
Copy link
Contributor

lanwen commented Feb 3, 2015

ok to test

* @return this builder
*/
public Builder maybe(final Builder regex) {
return this.group().add(regex.build().toString()).endGr().add("?");
Copy link
Contributor

Choose a reason for hiding this comment

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

add() can be used with builder. No need toString() method

@lanwen
Copy link
Contributor

lanwen commented Feb 3, 2015

Can I make new version after it?

@lanwen
Copy link
Contributor

lanwen commented Feb 3, 2015

think no need other methods - any of regex can be inserted with help of add(builder) method. Its overkill to implement each method to do so

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 89cfa80 on Tavio:maybe_with_builder into * on VerbalExpressions:master*.

1 similar comment
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 89cfa80 on Tavio:maybe_with_builder into * on VerbalExpressions:master*.

@Tavio
Copy link
Author

Tavio commented Feb 5, 2015

I think it would be nice to have the same functionality for other methods, such as oneOf() or anythingBut(), for instance. Should a user want to use a complex regex inside anythingBut(), for instance, they would have to do something like:

regex().add("(?:[^").add(OTHER_BUILDER_HERE).add("]*)");

Since it is the purpose of this library to facilitate the building and comprehension of expressions, it would be much nicer if the user could simply do:

regex().anythingBut(OTHER_BUILDER_HERE);

Unless, of course, there is a simpler way of doing this solely with the add() method which I'm not seeing.

@lanwen
Copy link
Contributor

lanwen commented Feb 5, 2015

ok, you convinced me

lanwen added a commit that referenced this pull request Feb 5, 2015
maybe() method now accepts a builder as parameter
@lanwen lanwen merged commit 1253395 into VerbalExpressions:master Feb 5, 2015
@nykolaslima
Copy link

Wouldn't be nice to add some example with maybe in the README? @Tavio @lanwen

@Tavio
Copy link
Author

Tavio commented Feb 5, 2015

Will do!

@matanox
Copy link

matanox commented Dec 3, 2017

Actually, I'm not seeing the following form of usage suggested (given the green light?) above, implemented in the code

regex().anythingBut(OTHER_BUILDER_HERE);

What am I missing?

@lanwen
Copy link
Contributor

lanwen commented Dec 3, 2017

This is simply not implemented yet.

@matanox
Copy link

matanox commented Dec 3, 2017 via email

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants