search

Reading-eScience-Centre / edal-java

Environmental Data Abstraction Layer libraries
Other
39 stars 30 forks source link

Add serialization required for terracotta #92

Closed yosoyjay closed 7 years ago

yosoyjay commented 7 years ago

Changes to support serialization required for terracotta.

guygriffiths commented 7 years ago

Code all looks fine, but it's introduced a lot of compiler warnings. We always explicitly add the serialVersionUID field for serializable classes (see https://stackoverflow.com/questions/285793/what-is-a-serialversionuid-and-why-should-i-use-it for an explanation of why it's good practice). There are 57 warnings associated with that - i.e. 57 classes which need serialVersionUID to be added. We just make it 1L, then increment it as other fields are added to the class.

There are 2 classes (AbstractDiscreteFeature and DiscreteFeature) where you have added the java.io.Serializable import, but never actually used it.

Finally, could you remove the @SuppressWarnings("unchecked") at line 97 of HorizontalMesh4dDataset - your new code makes this unnecessary.

yosoyjay commented 7 years ago

Thanks Guy, I'll add the serialVersionUID and make the requested changes.

yosoyjay commented 7 years ago

In order to avoid these types of issues in the future, I'd like to verify the commits pass without warnings your compiler is giving. What setup (compiler, warning settings, etc.) are you using for development?

I've now checked specifically for "Serializable class without 'serialVersionUID'" and "Serializable class with unconstructable ancestor" hoping that it matches your setup.

guygriffiths commented 7 years ago

Thanks, looks good. We're using the Oracle Java 8 compiler. Warning settings are just the defaults in eclipse. There is a list here: https://eclipse.org/tm/development/compiler_warnings.php which lists most of them. Note that that list is recommended settings for a specific project. The entries in bold have been changed from their defaults. A Warning in bold can be ignored, and an Error in bold should be a warning.

yosoyjay commented 7 years ago

Great, I'll be sure to use the same warnings and errors. Thanks!