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 nil check for user type in gen_app #1073

Merged
merged 4 commits into from Feb 25, 2017

Conversation

Projects
None yet
3 participants
@tchssk
Member

tchssk commented Feb 24, 2017

Fix #1060.

@tchssk

This comment has been minimized.

Show comment
Hide comment
@tchssk

tchssk Feb 24, 2017

Member

Would you try this to your project? @luna-duclos

Member

tchssk commented Feb 24, 2017

Would you try this to your project? @luna-duclos

@luna-duclos

This comment has been minimized.

Show comment
Hide comment
@luna-duclos

luna-duclos Feb 24, 2017

Contributor

Just tested this, code looks good, and code works! 👍

Contributor

luna-duclos commented Feb 24, 2017

Just tested this, code looks good, and code works! 👍

@raphael

This comment has been minimized.

Show comment
Hide comment
@raphael

raphael Feb 24, 2017

Member

Thank you for the great PR with additional tests! I am wondering though if it wouldn't be better to put the test in the caller (https://github.com/goadesign/goa/blob/master/goagen/codegen/publicizer.go#L130) rather than in the Publicize function itself. Publicize should not be called on nil and protecting against that could hide other bugs (where the target should not have been nil but was because of a bug).

Member

raphael commented Feb 24, 2017

Thank you for the great PR with additional tests! I am wondering though if it wouldn't be better to put the test in the caller (https://github.com/goadesign/goa/blob/master/goagen/codegen/publicizer.go#L130) rather than in the Publicize function itself. Publicize should not be called on nil and protecting against that could hide other bugs (where the target should not have been nil but was because of a bug).

@luna-duclos

This comment has been minimized.

Show comment
Hide comment
@luna-duclos

luna-duclos Feb 24, 2017

Contributor

I think a function not blowing up on nil is generally good behaviour. I can see your point about masking other bugs however, but considering these bugs would usually be the same nil check .. I don't see the harm

Contributor

luna-duclos commented Feb 24, 2017

I think a function not blowing up on nil is generally good behaviour. I can see your point about masking other bugs however, but considering these bugs would usually be the same nil check .. I don't see the harm

@raphael

This comment has been minimized.

Show comment
Hide comment
@raphael

raphael Feb 24, 2017

Member

For example you may end up with code that is generated fine but does not compile because the Publicize method is missing. This is a lot harder to track than a clear error such as the one you hit where it took me 10 seconds to find the offending line. In general I dislike the idea of writing defensive code "just because" - it makes tracking issues down the line orders of magnitude more difficult.

Member

raphael commented Feb 24, 2017

For example you may end up with code that is generated fine but does not compile because the Publicize method is missing. This is a lot harder to track than a clear error such as the one you hit where it took me 10 seconds to find the offending line. In general I dislike the idea of writing defensive code "just because" - it makes tracking issues down the line orders of magnitude more difficult.

@tchssk

This comment has been minimized.

Show comment
Hide comment
@tchssk

tchssk Feb 25, 2017

Member

I reverted the commit and moved nil check to publicizer for hash. Is it correct?

Member

tchssk commented Feb 25, 2017

I reverted the commit and moved nil check to publicizer for hash. Is it correct?

@raphael

This comment has been minimized.

Show comment
Hide comment
@raphael

raphael Feb 25, 2017

Member

Yes perfect, thank you for making the change!

Member

raphael commented Feb 25, 2017

Yes perfect, thank you for making the change!

@raphael raphael merged commit 934c228 into goadesign:master Feb 25, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

raphael added a commit that referenced this pull request Mar 1, 2017

Add nil check for user type in gen_app (#1073)
* Add tests for UserTypeWriter in gen_app

* Add nil check for user type in gen_app

* Revert "Add nil check for user type in gen_app"

This reverts commit ecf0f68.

* Add nil check in publicizer for hash

raphael added a commit that referenced this pull request Mar 1, 2017

Add nil check for user type in gen_app (#1073) (#1087)
* Add tests for UserTypeWriter in gen_app

* Add nil check for user type in gen_app

* Revert "Add nil check for user type in gen_app"

This reverts commit ecf0f68.

* Add nil check in publicizer for hash
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment