Added ReadonlyRootfs option. by scadgek · Pull Request #245 · docker-java/docker-java
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
boolean of Boolean?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't this cause problems when property wouldn't exist? Boolean will default to null, while boolean may report false negative 'false'.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I think we should change all json mapped boolean fields into Boolean in a separate refactoring.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just interesting because i see inconsistency everywhere. Maybe document somewhere DESIGN.md ?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What inconsistencies did you find. Some examples?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
boolean vs Boolean in other places, but i may mistake.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, in this case a default boolean = false will be ok as long as ReadonlyRootfs option is false by default. I think it'll we better to add default values in such classes corresponding to docker defaults, than creating objects and handling nulls in several places.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Best will be to use object types instead of primitives. See #246