Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add serialization required for terracotta #92

Conversation

yosoyjay
Copy link
Contributor

Changes to support serialization required for terracotta.

@guygriffiths
Copy link
Contributor

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
Copy link
Contributor Author

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

@yosoyjay
Copy link
Contributor Author

yosoyjay commented Jun 12, 2017

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
Copy link
Contributor

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.

@guygriffiths guygriffiths merged commit a9d75aa into Reading-eScience-Centre:develop Jun 13, 2017
@yosoyjay
Copy link
Contributor Author

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

@yosoyjay yosoyjay deleted the add_serialization_required_for_terracotta branch June 13, 2017 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants