search

dwins / mapnik2geotools

Using the Scala XML API to translate from Mapnik XML to GeoTools' SLD dialect
64 stars 22 forks source link

Merge before Mapnik 0.7 support changes? #29

Closed nrenner closed 12 years ago

nrenner commented 12 years ago

Hi David,

in case you would like to add my changes to your project, I thought it might be easier to do so before you start your work on the Mapnik 0.7 support. The task is not finished yet, but the color format and label issues are. But we could as well wait until the task is done and merge afterwards - just as you please.

I'm new to Scala, so please review and feel free to suggest anything I can improve before merging. I've done some regression tests by comparing SLD outputs from your jar and my version and deployed the osm.xml and the old mapquest-us.xml to GeoServer and all looked OK to me.

I'm not going to work on m2gt over the weekend.

Regards, Norbert

dwins commented 12 years ago

These patches look fine really but since you're new to Scala and asked for suggestions I went ahead and tweaked things a bit to be more idiomatic to the language, being careful to make small commits so you can follow along in the change history if you like. The basic things are use of Scala's Option class instead of nullable variables and avoiding mutable variables too (prefer val over var where possible.) I didn't change any behavior though.

If you're interested in reading up on Option, http://www.codecommit.com/blog/scala/the-option-pattern is a good place to start.

nrenner commented 12 years ago

Thanks a lot for your hints and the step-by-step commits! I find that really helpful.