search

iamthechad / grails3-recaptcha

ReCaptcha plugin for Grails 3
Other
3 stars 10 forks source link

upgrade plugin itself to grails 3.2.4 and cleanup #18

Closed snimavat closed 7 years ago

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-0.04%) to 93.452% when pulling b05fdd3dbfee36f6139f2a5f0ce22e0cc8e0a583 on snimavat:3.2-upgrade into c9dc71faa3abbca0c619d5a0ccb07f920f00057d on iamthechad:v3.x.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-0.04%) to 93.452% when pulling b05fdd3dbfee36f6139f2a5f0ce22e0cc8e0a583 on snimavat:3.2-upgrade into c9dc71faa3abbca0c619d5a0ccb07f920f00057d on iamthechad:v3.x.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-0.04%) to 93.452% when pulling 38479248a79e8e566cc670bf6bb67f0c1891c1b4 on snimavat:3.2-upgrade into c9dc71faa3abbca0c619d5a0ccb07f920f00057d on iamthechad:v3.x.

iamthechad commented 7 years ago

Thanks for this submission. Please take a look at the contribution guidelines.

This project prefers to have issues opened first, even for PRs, in order to facilitate discussion.

In particular, I'd like to know: what problems does this PR solve?

This PR also seems to be targeting several different things - plugin upgrade, Travis CI config, "cleanup". Ideally, these separate changes should be handled in separate PRs.

snimavat commented 7 years ago

@iamthechad the goal was to upgrade to latest grails, clean up build... i also cleanedup few things with travis, but no behavior is changed.

If you could merge it to a temp branch, and if u think it needs any changes thn if u can do it or ask me to do it and thn finally merge to main branch and release, that would be great..

I want to upgrade spring-security-recaptcha to grails 3 but there's no wy i can compile the spring-sec-recaptcha with current version of recaptcha plugin it fails with some groovy error complaning about malformedtype

iamthechad commented 7 years ago

Is there a need to upgrade to the latest Grails? Does this solve a problem?

Is there a need to modify the Travis configuration? Does this solve a problem?

There were also a lot of changes to code to change from dynamic typing to static typing. Does this solve a problem?

snimavat commented 7 years ago

Upgrading to latest grails helps - yes Travis, no, this was just for cleaning up and enabling gradle caches Compile static - thts just cleanup, i think static compilation is preffered unless we need it for code to function.. that doesnt change any behavior though and all tests pass...

iamthechad commented 7 years ago

Then what is needed is 1 PR for just the Grails upgrade that references the original issue (#17). For what it's worth, I was completely unable to reproduce the issue noted in #17, so more information is needed before I'll be convinced that the upgrade is needed.

A new issue for the Travis changes can also be opened, since this changes are separate.

A new issue can also be opened for the dynamic to static type changes, and the merits of such a change can be discussed there.

I am not comfortable merging this PR with the number and type of changes.

snimavat commented 7 years ago

@iamthechad sure, will give seperate PRs for each and will have a ticket for them.. The code which fails to compile with current version is here https://github.com/snimavat/recaptcha-spring-security/tree/grails3 (still in progres) just in case if you are curious to try it.