search

mangstadt / biweekly

biweekly is an iCalendar library written in Java.
BSD 2-Clause "Simplified" License
323 stars 44 forks source link

Fields in a Serializable class should either be transient or serializable #32

Closed zeeshanasghar closed 8 years ago

zeeshanasghar commented 8 years ago

This pull request is focused on resolving occurrences of Sonar rule

squid:S1948 Fields in a Serializable class should either be transient or serializable

You can find more information about the issue here: https://dev.eclipse.org/sonar/rules/show/squid:S1948

Please let me know if you have any questions.

Zeeshan Asghar

codecov-io commented 8 years ago

Current coverage is 77.45%

Merging #32 into master will not affect coverage as of 8713f33

@@            master     #32   diff @@
======================================
  Files          252     252       
  Stmts        10157   10157       
  Branches      1971    1971       
  Methods                          
======================================
  Hit           7867    7867       
  Partial        400     400       
  Missed        1890    1890

Review entire Coverage Diff as of 8713f33

Powered by Codecov. Updated on successful CI builds.

mangstadt commented 8 years ago

I don't know why you decided to make just 3 classes Serializable. And I don't know why you made the messages field transient in the Messages class.

zeeshanasghar commented 8 years ago

Messages.java contains an enum and Java Enums Are Inherently Serializable that why i create private final ResourceBundle messages; as transient.

Other Serialized change is due to that they are used in already Serialable classes in 

CannotParseException
ICalTimeZone
SkipMeException
ICalDate
NotPredicate<T>, AndPredicate<T>, OrPredicate<T>
mangstadt commented 8 years ago

Oh, I see. Thank you for the explanation. There are some changes I want to make to your pull request, so I will commit these changes myself. Thanks again.

mangstadt commented 8 years ago

I am going to skip making VTimezone serializable, because this would require all ICalComponent and ICalProperty classes to also be serializable. The reason for this is that VTimezone extends the ICalComponent class, which contains Map objects that hold arbitrary ICalComponent and ICalProperty instances. I feel like making all of the property and component classes serializable would add unnecessary bloat to the API.

Changes committed in: 61a14f05a04ea105125f9403ce019c8edaf1cbfc

zeeshanasghar commented 8 years ago

ok, thanks for consideration. you can close this PR and merged it manually as you desired. also put the comment that I merged it manually.

mangstadt commented 8 years ago

Zeeshan,

I merged the changes in 61a14f05a04ea105125f9403ce019c8edaf1cbfc. Thanks again.