[GitHub] commons-text pull request #55: TEXT-97: RandomStringGenerator able to pass m...

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] commons-text pull request #55: TEXT-97: RandomStringGenerator able to pass m...

mureinik
GitHub user ameyjadiye opened a pull request:

    https://github.com/apache/commons-text/pull/55

    TEXT-97: RandomStringGenerator able to pass multiple ranges to .withinRange()

    *.withinRange()* now able to accept multiple ranges.
   
    Ex.
    ```
    final char [][] pairs = {{'a','z'},{'0','9'}};
    RandomStringGenerator generator = new RandomStringGenerator.Builder().withinRange(pairs).build();
    ```

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/ameyjadiye/commons-text TEXT-97

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/commons-text/pull/55.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #55
   
----
commit beb0b4615a94420cc86595c1f060711dd999af91
Author: Amey Jadiye <[hidden email]>
Date:   2017-07-01T14:55:35Z

    RandomStringGenerator able to pass multiple ranges to .withinRange()

----


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] commons-text issue #55: TEXT-97: RandomStringGenerator able to pass multiple...

mureinik
Github user coveralls commented on the issue:

    https://github.com/apache/commons-text/pull/55
 
   
    [![Coverage Status](https://coveralls.io/builds/12217890/badge)](https://coveralls.io/builds/12217890)
   
    Coverage increased (+0.007%) to 97.312% when pulling **beb0b4615a94420cc86595c1f060711dd999af91 on ameyjadiye:TEXT-97** into **5e479dcd74dab262e5080991796395c3e29222b9 on apache:master**.



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] commons-text issue #55: TEXT-97: RandomStringGenerator able to pass multiple...

mureinik
In reply to this post by mureinik
Github user ameyjadiye commented on the issue:

    https://github.com/apache/commons-text/pull/55
 
    @chtompki , @PascalSchumacher , can you please express your opinion on this , please check JIRA for detail discussion.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] commons-text issue #55: TEXT-97: RandomStringGenerator able to pass multiple...

mureinik
In reply to this post by mureinik
Github user chtompki commented on the issue:

    https://github.com/apache/commons-text/pull/55
 
    Sure...I'll try to look at this in the coming day or so.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] commons-text issue #55: TEXT-97: RandomStringGenerator able to pass multiple...

mureinik
In reply to this post by mureinik
Github user jbduncan commented on the issue:

    https://github.com/apache/commons-text/pull/55
 
    @ameyjadiye I don't know if there's a convention in Apache Commons for preferring `new X.Builder()` over static factory methods like `X.builder()`, but if there isn't, then I would encourage changing the API so that instead of `new RandomStringGenerator.Builder()`, one can write the shorter and arguably more readable `RandomStringGenerator.builder()`.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] commons-text issue #55: TEXT-97: RandomStringGenerator able to pass multiple...

mureinik
In reply to this post by mureinik
Github user chtompki commented on the issue:

    https://github.com/apache/commons-text/pull/55
 
    @jbduncan has a point here, but the code here does conform to the style of the existing code. So, I'd lean more towards the changes @ameyjadiye is proposing mainly because to re-work the code into a static `builder()` method pattern would require changes that would necessitate a major version change. We could, though, revisit this in the 2.X version if you with @jbduncan. Thoughts?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] commons-text issue #55: TEXT-97: RandomStringGenerator able to pass multiple...

mureinik
In reply to this post by mureinik
Github user jbduncan commented on the issue:

    https://github.com/apache/commons-text/pull/55
 
    Revisiting this in 2.x sounds good to me!
   
    The only other thought I have is that I think this should go up as a new issue on Apache JIRA if it hasn't already.
   
    Other than that, everything LGTM.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] commons-text issue #55: TEXT-97: RandomStringGenerator able to pass multiple...

mureinik
In reply to this post by mureinik
Github user ameyjadiye commented on the issue:

    https://github.com/apache/commons-text/pull/55
 
    Yeah, will see if we can simplify API in 2.x .
    I need opinion on the method I have given in this PR, According to me it's good addition to existing API but go through JIRA discussion once.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Loading...