afpdev / alpheusafpparser

Library & parser for IBM Advanced Function Presentation (AFP) document/print stream format
GNU General Public License v3.0
16 stars 11 forks source link

Various enhancements and some minor bug fixes #11

Closed michaelknigge closed 8 years ago

michaelknigge commented 8 years ago

I've implementes some minor enhancements, clode cleanups and bug fixes. More is to come.....

michaelknigge commented 8 years ago

Rudolph,

move it to "examples" directory.

You mean the CountPageGroupsAndPagesInPageGroup class, don't you?

don't delete it. Both, COPYING and LICENSE, are there for a purpose.

ok.

remove the eclipse plugin.

why? It has no impact on the build process. But it gives the developer (and contributor) that uses Eclipse more power. A simple "gradle eclipse" generates all required project files for eclipse and the developer can start contributing. I do not see any reason why you really want to make a contributors life harder. I also have on my list to add a Eclipse configuration file that makes sure that a developer can only write code that complies to the coding guidelines that are checked by checkstyle. But before I add this I'll make sure that the code is well formatted so checkstyle will not mark any piece of code. But this may be a hard way... As you may remember, user quike has added Checkstyle, but running Checkstyle currently marks 3395(!!!!!) lines of code! I really don't understany why he has added the Checkstyle plugin but has not cleaned up the code....

lol. No. We keep the XML writer.

Ok, but if it is a valuable piece of code than I would suggest that we should let it look so! The source has no JavaDoc and no (usable) main method. I agree that a "AFP-to-XML" printer may be useful, but than it should have a main() so it can be used directly by a user. And it should contain a JavaDoc and it should not be that "hidden" ;-) We should then add a hint in the README.md to that tool. You may also think about if laughing at a contributor is polite...

please add more specific information to the fail messages where exactly in the AFP file the exception happened.

What concrete commit you are talking about? I've tried to give the user not more, but also not less information that was available before my changes.

The source 1.7. However, I see no need to have the build tools or the target JVM to remain on 1.7. Do you?

Yes ;-) You have specified sourceCompatibility = '1.7' in the Gradle build file. This tells the Java compiler that the source may only contain syntax that is compatible with Java 1,7 (for example if you compile with Java 1.8 and use a lamda expression, than the compile would fail). BUT if you use a Java 1.8 compiler you may use classes and methods that are only available with Java 1.8 and you would probably not recognize - as long as you still code in Java 1.7 syntax, the compile would not fail. This is the reason why I've downgraded the checkstyle version so the developer can (or better: SHOULD!) use Java 1.7 to make sure that the code does not use stuff that is available only in Java 1.8 or newer. And to think about the future: If you will upload the JAR to Maven Central you should make 100% sure that the JAR is compatible with Java 1.7 so the JAR is of use to Java 7 developers, too.

Could you please write tests for every SF, triplet, and repeating group first?

Which commit are you talking about? Addition of Jacoco? JUnit and system tests are on my list. But to get feedback if my JUnit and system test are testing code that is currently untested (which is currently over 90%) I need jacoco. Beside that: IMHO adding JUnit tests to YOUR software is YOUR job. If a contributor adds new code than of course the contributor has to supply JUnit tests, too. But asking a contributor to add JUnit tests to code that is written by YOU sounds like kidding (or just delegating your work to other people).... But maybe I've got you wrong....

michaelknigge commented 8 years ago

I've added some new stuff. Additionally my suggestion is to move the AFP2XML class to the new examples package too and make a "real" console application of it).

Furthermore I'd like to rename the class so it does look more like existing Java classes (use camel case like "AfpToXml") and do the same with "CountPageGroupsAndPagesInPageGroup" (rename to something like "AfpPageCounter")....

What do you think? I'd so so if you'd signal that you'll accept the changes...

michaelknigge commented 8 years ago

The Visitor Pattern is no Antipattern - it is one of the best patterns around!

I'm currently working on an AFP dump utility that displays all fields of the structured fields using reflection. Using this way, I do not have to care about the nnn concrete implementations of the structured fields. I only need to care about the few implementations StructuredFieldBaseName, StructuredFieldBaseNameAndTriplets, StructuredFieldBaseRepeatingGroups,...

If I use the visitor I can be 100% sure that my dump utility handles all possible structured fields. This would not be possible by using instanceof - because then I would have to implement a "dumper" for every structured field class.

Furthermore: If you'll add a new extendsion of StructuredField AND also add this to the visitor, the next compile of my AFP dumper will fail because my implementation of the visitor is missing a method. This makes sure that my application always handles all possible structured fields.

This is no antipattern - this is just save...

But regardless if you like the visitor pattern or not - IMHO a library should provide a visitor so the user of the library can decide if he/she uses the visitor or instanceof....

michaelknigge commented 8 years ago

If something is missing that is a blocker for my pull request let me know.... After the pull request is accepted and added I hope that I'll find some time to care about the checkstyle violations.... >3000 is way too much....

codecov-io commented 8 years ago

Current coverage is 7.51% (diff: 2.89%)

No coverage report found for master at 1c8ec2f.

Powered by Codecov. Last update 1c8ec2f...57f4958

michaelknigge commented 8 years ago

SORRY..... Please ignore the last two commits.... I'll revert them of course.... I've committed them to the wrong branch....

mogozine commented 8 years ago

Hi Michael! Thanks for this pile of changes. I think we have vastly different views on this project. You are allowed to use and change Alpheus under the GPL3 license. You don't have to send pull requests in the future. I will pull from your branch where I see a need for it.

michaelknigge commented 8 years ago

Rudolf,

sadly.... very sadly.... I'm a little bit confused... You've asked me to add some stuff (i. e. the getter for nrOfBuilt object) and after I've done so you tell me that you don't wanna have any further pull request. I'm confused.

But if this is your wish - ok. It's sadly because this supports (or even "enforces") fragmentation of Alpheus.

I'll think about it. I guess I'll create a new project on GitHub with a new name and use Alpheus as its base. So we will have no naming conflicts if we both upload our projects to i. e. maven central...

Have a good time, Michael