search

alexruiz / fest-assert-2.x

FEST Fluent Assertions 2.x
http://fest.easytesting.org
Apache License 2.0
401 stars 69 forks source link

Github#71 implementation #72

Closed olivierdemeijer closed 12 years ago

olivierdemeijer commented 12 years ago

Hi, I followed the instructions you give in "Contributors guideline", so this should be ok (doc, test, format, ...). However, i'm not familiar at all with github, so I hope i didn't screw too much thing. Olivier

Change-Id: Ie547fd0c7bc13de0aaf03ae5d6672cce997ed833

joel-costigliola commented 12 years ago

Hi Olivier,

I'm afraid you did screw things a little bit ;-) it seems you have formatted all the files of the project ! Whet IDE did you use Eclipse, Ideas ... another one ?

Our formatting preferences have been defined for eclipse first, and ported to Idea few days ago, the Idea formatting preferences are not perfect and needs to be closer to the eclipse one, it may explain this if you used Idea (sorry if it's the origin of the problem).

I can't integrate your work right now because it is almost impossible for me to know what have really changed.

But the situation is not as bad as it sounds, you just have to do the same work without formatting all the sources, it should not be too complicated.

What I would do if I were you :

Thanks for contributing, we appreciate that !

Joel

olivierdemeijer commented 12 years ago

Hi Joel, Don't be afraid :) I still don't understand why everything single file was formatted (save for the new ones i created, i didn't open more than a dozen), ... knowing that before importing the project in Eclipse, i switched off my save action settings and then import @project level the formatting preferences you provide. Weirdest, a comparison between files on master and files I committed shows that everything is different ... but paying a close look, there is, visually speaking, no difference at all. I screwed, ... but don't know where yet :) (encoding ? I use UTF-8, don't know what is project preference).

No matter, i'll try the solution you suggest. Olivier

On Fri, Jul 6, 2012 at 10:10 AM, Joel Costigliola < reply@reply.github.com

wrote:

Hi Olivier,

I'm afraid you did screw things a little bit ;-) it seems you have formatted all the files of the project ! Whet IDE did you use Eclipse, Ideas ... another one ?

Our formatting preferences have been defined for eclipse first, and ported to Idea few days ago, the Idea formatting preferences are not perfect and needs to be closer to the eclipse one, it may explain this if you used Idea (sorry if it's the origin of the problem).

I can't integrate your work right now because it is almost impossible for me to know what have really changed.

But the situation is not as bad as it sounds, you just have to do the same work without formatting all the sources, it should not be too complicated.

What I would do if I were you :

  • save you current somewhere, let's to say fest-save directory
  • on the master branch, reset to the origin/master (should be the previous commit) to restart the work on a clean state
  • take the files you edited/created and put them in merge them manually to master
  • check the formatting
  • once it seems good, let me know I'll be happy to review your work

Thanks for contributing, we appreciate that !

Joel

Reply to this email directly or view it on GitHub: https://github.com/alexruiz/fest-assert-2.x/pull/72#issuecomment-6799548

gesellix commented 12 years ago

Probably some change between spaces and tabulators? Sometimes there are some spaces at line ends, too, which could be removed by some formatter, but aren't visible in some diff tools. My proposal would be to completely apply the code style on all files in the upstream repositories so that we can be sure on which side of the patch the "better" format is.

joel-costigliola commented 12 years ago

@olivierdemeijer I finally had some time to review your work in more details, first a good news : the diff problem seems to appear in github only and your last commit (4057ed7267373746aac7309260708831177d38dc) seems ok, I had no problem comparing it to my master branch. The bad news : it does not compile because of canExecute which only exist in java 6 but not in java 5. The trouble here is that maven does not assure us of using java 5 only classes even though we have <source>1.5</source> and <target>1.5</target> in our pom.xml. Eclipse does a better job as it has signaled the errors.

As we have not decided yet to go java 6 only, could you remove canExecute related code (or better move it to another branch for later usage)

By the way, I did get caught with that kind of incompatibility issue ;-)

ps : I'm gonna create an issue to improve our maven build.

olivierdemeijer commented 12 years ago

I played a little with github during week-end, and reached the same conclusion : diff tool was somehow "broken" (was unable to find anyone complaining about this, however. Rather strange). Agree on moving canExecute to another (temporary) branch, i'll do it asap.

joel-costigliola commented 12 years ago

@olivierdemeijer I have improved the maven build, it now detects Java 5 API violation, you can now rely on it. I also updated a little bit the formatting rules and reformatted all our code accordingly - don't be surprised ;-)

joel-costigliola commented 12 years ago

canRead() and canWrite() integrated, thanks !

olivierdemeijer commented 12 years ago

Hi Joel, Sorry for very long delay for me to answer (pre-holidays pressure @work ... then holidays ;)) and thx for fixing those problems. Expect more commits from my part, as fest-assert is a really convenient tool !! Olivier

On Sun, Jul 29, 2012 at 4:41 PM, Joel Costigliola < reply@reply.github.com

wrote:

canRead() and canWrite() integrated, thanks !

Reply to this email directly or view it on GitHub: https://github.com/alexruiz/fest-assert-2.x/pull/72#issuecomment-7353436

joel-costigliola commented 12 years ago

Hi Olivier,

No problem, holidays are a very good reason :) It is funny timing, Alex has just decided to migrate to Java 6, it means we can add canExecute assertion. Can I let you take care of this ? - a small contribution is kind of ideal for those who come back from holidays :)

Cheers,

olivierdemeijer commented 12 years ago

Hi Joel, Good idea :) Question : where are fest-util 1.2.3 SNAPSHOT and fest-test 2.1.0 SNAPSHOT published ? This prevents me to build fest-assert-core ... and to enjoy working on it after well deserved holidays :) Olivier

On Tue, Aug 21, 2012 at 9:34 PM, Joel Costigliola notifications@github.comwrote:

Hi Olivier,

No problem, holidays are a very good reason :) It is funny timing, Alex has just decided to migrate to Java 6, it means we can add canExecute assertion. Can I let you take care of this ? - a small contribution is kind of ideal for those who come back from holidays :)

Cheers,

— Reply to this email directly or view it on GitHubhttps://github.com/alexruiz/fest-assert-2.x/pull/72#issuecomment-7913198.

joel-costigliola commented 12 years ago

Hi, We don't publish snapshot versions yet so you've got to build them by yourself (build also fest parent pom in project https://github.com/alexruiz/fest-maven-setup).

Joel

olivierdemeijer commented 12 years ago

Ok, i'll get the whole project then. Thx, Olivier

On Thu, Aug 23, 2012 at 2:08 PM, Joel Costigliola notifications@github.comwrote:

Hi, We don't publish snapshot versions yet so you've got to build them by yourself (build also fest parent pom in project https://github.com/alexruiz/fest-maven-setup).

Joel

— Reply to this email directly or view it on GitHubhttps://github.com/alexruiz/fest-assert-2.x/pull/72#issuecomment-7967521.