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

Refactor InvokerHelper formatting methods #96

Closed
wants to merge 6 commits into from
Closed

Refactor InvokerHelper formatting methods #96

wants to merge 6 commits into from

Conversation

tkruse
Copy link
Contributor

@tkruse tkruse commented Aug 21, 2015

These are some refactorings and minor enhancements of the formatting methods in InvokerHelper.

The formatting of maps, arrays, collections and ranges should be more consistent, allowing the same parameters (verbose, maxSize, safe) everywhere. The 'safe' parameter gets used in many more contexts (though default is still false).

I hope that existing behavior and API was not changed (too much). I believe I did not change any existing public method signature, though I added a few with more options.

The behavior should be the same, except for the case when an Object.toString() method throws a checked exception, then this will be rethrown wrapped in a GroovyRuntimeException, when a method is invoked with 'safe=true', which I believe is done by PowerAsserts.

Some changes might need further discussions. One thing annoying me still is that after this PR, the methods
toArrayString(Object[] collection, boolean verbose, int maxSize, boolean safe) and formatCollection(Collection collection, boolean verbose, int maxSize, boolean safe) are syntactically equal, not sure if there is a nice way not to replicate code, yet also not transforming arrays to collections or vice versa.

The preAllocation of the StringBuffers might be too big, maybe a value like 3 or 4 might be nicer. It's always a heuristic value, maybe premature optimization.

I hope next to change the default printing in PowerAsserts and Groovysh to use the "verbose" flag.

@tkruse tkruse changed the title Refactor invoker Refactor InvokerHelper formatting methods Aug 25, 2015
@tkruse
Copy link
Contributor Author

tkruse commented Aug 25, 2015

rebased and added another little fix to escaping (with test)

@blackdrag
Copy link
Contributor

on first look looks good to me... btw, very nice breaking up of commits and commit messages!

@PascalSchumacher
Copy link
Contributor

@tkruse: Could you please add a Apache license header to the test? Thanks!

@tkruse
Copy link
Contributor Author

tkruse commented Sep 1, 2015

Header added to new file. Note that there is some discussion on https://issuesPRE.apache.org/jira/browse/GROOVY-7564, so I guess this should not be merged until 7564 is okay with Paul, though I can make 7564 a separate PR if necessary. It is just one commit, independent of the rest.

Maybe a third opinion on this could help.

@tkruse
Copy link
Contributor Author

tkruse commented Sep 1, 2015

After reconsidering, reduced this PR to just providing strict refactorings, meaning (hopefully) no change of output.
The more controversial changes I have moved to #111

@tkruse
Copy link
Contributor Author

tkruse commented Sep 2, 2015

Added several more tests in a commit before the changes to show API and behavior remain the same (tests should run before and after the refactorings)

@paulk-asert
Copy link
Contributor

@tkruse
Copy link
Contributor Author

tkruse commented Sep 10, 2015

all PRs good now, I think. There were minor problems stemming from the reordering of commits to separate commits that refactor from commits that change behavior. Sorry that I don't retest locally after every rebase or commit change.

@tkruse
Copy link
Contributor Author

tkruse commented Oct 19, 2015

bump

@asfgit asfgit closed this in c9bd001 Jul 28, 2016
@paulk-asert
Copy link
Contributor

Merged (finally!!), thanks!

@tkruse tkruse deleted the refactor-invoker branch July 28, 2016 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants