Pull Request for the new Shrinkwrap Descriptors

[rbattenfeld]

Hi Andrew

I had to install a new git version on my Ubuntu laptop and additionally, a new repository. I followed this time your developer guidelines. I hope it is now correct:-) I understand now git and the workflow a little bit more.

The only thing which didn't work was to execute 'git push upstream master' because permission issues. That is the reason why I send a pull request to you.

Thanks again for the git instructions.

Regards, Ralf

[ALRubinger]

Interesting.

1) Why the new GitHub account? 2) I opened a JIRA specifically to track this large-scale work. https://issues.jboss.org/browse/SHRINKDESC-54 We already have quite a few issues open which this may address, so we'll comb through those and link as appropriate. 3) Probably you'll wanna rename your branch to SHRINKDESC-54 4) There are a lot of compiler warnings in the impl module due to non-read fields. The generator putting unnecessary stuff in place there? 5) This has been nicely rebased with upstream/master. So I'm going to push it to upstream/SHRINKDESC-54 as the upstream sync point for us to work. Recommend that looking forward, you keep your local stuff synced with upstream/SHRINKDESC-54, and I'll in turn ensure that I keep that upstream branch rebased off master so everything stays current. This will make it ultra-easy to merge into master when all's ready. 6) The inability to push to upstream/master is intentional. :) Pull requests are the right process; it enforces peer review and sometimes I'll do things like rebasing or squashing commits before putting things into the immutable upstream.

[ALRubinger]

7) The "gen" module is no longer part of the aggregator build. This is on purpose now?

[ALRubinger]

8) This really is a very compelling start and path forward. My biggest concern at the moment with putting it into upstream/master is code coverage. An easy way to see the gaps in test coverage is to use the Coburtura plugin for Eclipse, and do "Coverage As > JUnit". It'll run all the tests, create a tab with a report, and paint the impl classes red in places where we haven't run through those code paths.

[ALRubinger]

9) We need a couple things on all source files:

• JBoss / ASL2 Copyright
• JavaDoc at all Class level, usually with author name. This is typically accomplished by setting up templates in the IDE. Find me on chat if you need samples.

[ALRubinger]

Authoritative sync point for this development until it makes it into upstream/master:

https://github.com/shrinkwrap/descriptors/tree/SHRINKDESC-54

Again, I'll keep this rebased off upstream/master. And any commits we make to our individual forks should have pull requests issued to this branch for topic dev.

[rbattenfeld]

Thanks for your comments. I am comming late home this night but I will try to meet you on the chat channel. Below are short answers:

1) I had always troubles with the first repo. Password issues, GIT version on my ubuntu etc. Some of the instructions didn't work, probably because I didn't setup the repo as recommended in the Shrinkwrap guidelines. So, I first solved the GIT Ubuntu problem. Created the SSH keys and finally created a new fork on github und a local one following the guide lines. The second reason is, that the generation is not pending on a particular Shrinkwrap API and IMPL base classes. So, a rebase is not really required. It would be anyway accepting all changes in the core classes.

2) 3) Good, I will rename the branche to SHRINKDESC-54

4) I will look at this compiler warnings and try to remove such unused fields.

5) Yes, I will sync it with upstream/SHRINKDESC-54

6) Good to know, I am still learning all the GIT stuff and how to collaborate with you and all others :-)

7) The gen module is actually isolated from the API and IMPL modules. The xslt transformation as well as executing the thousends of test cases are done only in this module. The only reason to compile the gen module is when something in the xls scripts has changed.

8) Very good, I will install this tool and will run the report. Please be aware that most of the test cases are executed in the gen module. Here, almost all interfaces are tested. Thats it the reason for the long time to execute the test cases. I am wondering, if I have covered all... The test cases in the impl module are intended to check the link to the schema itself whereas the gen test cases are testing that the generated API match with the generated IMPL classes.

9) This is also something I thought about. When I am in the chat channel you can provide me with an example and I can include them into the transformation process easily.

So you this evening in the chat room. Thanks for your comments.

[rbattenfeld]

Hi Andrew

I worked out all the issues you have commented over the last days:

1. No compiler warnings
2. All files have the copyright and the author comment
3. All classes and methods are added with javadoc. The javadoc html files are generated manually
4. As suggested, I analysed the junit coverage with eCobertuna. I think the statistic is quite good. Almost 100% covered. In the gen module, about 1500 test cases are generated in addition to the API and IMPL classes.
5. I also added a reflection test case that tests for every public method null value arguments. I found to cases with a null pointer exception. This is fixed as well.

I wanted today updated the branch with master but I realised that in the meantime the core classes are modified and I had trouble to sinc first the master branch with upstream... I will try tomorrow to solve this. May I ask you again stupid question about git in the chat channel:-(

[ALRubinger]

Ralf, one note about Git:

When I push rebased changes up to upstream/SHRINKDESC-54, I force changes (because we've rewritten history). What you need to do to bring your stuff current is to is force the pull:

"git pull -f".

The reasoning for this: We're constantly rebasing off master. Which means that our work will always be applied atop master. In order to do this, we have to reorder the commits we make, and force rewriting. So to pull in changes you'll have to tell git that yes, this is OK to do.

[ALRubinger]

Closing this as it's now been taken care of by more recent work.