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

Adding ability to externalize config and use string booleans. #33

Merged
merged 2 commits into from
Jan 22, 2015
Merged

Adding ability to externalize config and use string booleans. #33

merged 2 commits into from
Jan 22, 2015

Conversation

ctoestreich
Copy link

Also doing cyclomatic complexity cleanup on code

@stokito
Copy link
Owner

stokito commented Jan 22, 2015

Hi @ctoestreich
Thanks for your pull request, it's looks cool. Travis Ci says that test are failed https://travis-ci.org/stokito/grails-cookie
Could you check why?
I can do it myself, if you not interested

@stokito stokito self-assigned this Jan 22, 2015
stokito added a commit that referenced this pull request Jan 22, 2015
Adding ability to externalize config and use string booleans.
@stokito stokito merged commit 24eb0bb into stokito:master Jan 22, 2015
@stokito
Copy link
Owner

stokito commented Jan 23, 2015

I reverted some small changes:

  1. return keyword is always explicit to avoid errors during refactorings
  2. writeCookieToResponse() doesn't change cookie value, so it should be void
  3. getDefaultCookiePath(String path = '') was reverted because this default param value never used.

@ctoestreich
Copy link
Author

Thanks for adding so quickly. The changes you made above make sense. I noticed that the grails required version was bumped to 2.4 > *. This is not exactly the case. I tested this with 2.2.1, 2.3.8 and 2.4.0 and all versions of grails > 2.2 were able to use the plugin. I reverted the required version back to 2.2 to allow broader use of the plugin. Thanks!

@stokito
Copy link
Owner

stokito commented Jan 23, 2015

Yep, but problem was with running unit tests, they failed in grails 2.2.0

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.

2 participants