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.

Sign up for GitHub

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

Jump to bottom

Fix Nullability warning in JsonAdapters #78

Merged
merged 1 commit into from
Jan 2, 2020
Merged

Fix Nullability warning in JsonAdapters #78

merged 1 commit into from
Jan 2, 2020

Conversation

cortinico
Copy link
Collaborator

Updating generated methods toJson to accept a non null JsonWriter.
Previously the IDE was raising an issue due to different method signature.

Fixes #74

Ping @macisamuele for the review

@cortinico cortinico mentioned this pull request Dec 26, 2019
Closed
@macisamuele
Copy link
Collaborator

@cortinico please fix tests first.
Additionally this PR seems including some changes (operation id header) of #79 ... please make sure to make those changes only once ;)

@cortinico
Fix Nullability warning in JsonAdapters
c2f416d 
Updating generated methods `toJson` to accept a non null `JsonWriter`.
Previously the IDE was raising an issue due to different method signature.

Fixes Yelp#74
@cortinico cortinico force-pushed the fix-74-nullability-in-json-adapters branch from 64ce597 to c2f416d Compare January 1, 2020 21:01
@cortinico
Copy link
Collaborator Author

cortinico commented Jan 1, 2020 •
edited

ping @macisamuele for another pass of review (for some reasons I can't add you as reviewer).

I've also rebased on top of master and now the diff is cleaner. We still have some changes that are not completely related. The reason is that code inside the generated-code module was never re-generated in the past. I'm doing it in c2f416d. I can separate it if needed.

macisamuele
macisamuele approved these changes Jan 2, 2020
View reviewed changes
Copy link
Collaborator

@macisamuele macisamuele left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for cleaning the diff.
About the changes in generated-code, that's ok to me.
It's highly probable that the update was forgotten.

Do you think that we could have a test that can help us to avoid such conditions?
We might have a test that is allowed to fail such that development is not slowed down. Having the directory in helps to "visualise" what type of code will be generated and it would be great to have it top-notch

@macisamuele macisamuele merged commit e9b4a14 into Yelp:master Jan 2, 2020
@cortinico cortinico deleted the fix-74-nullability-in-json-adapters branch January 2, 2020 11:40
@cortinico
Copy link
Collaborator Author

Do you think that we could have a test that can help us to avoid such
conditions?

So ideally we could:

  • Remove the generated-code sample. The idea of this sample was to commit the generated code. Now that we have junit-test, the former is probably not needed anymore and is yet another thing to maintain.
  • Have a pre-commit hook that rebuilds everything before every commit.

@cortinico cortinico added this to the 1.3.0 milestone Jan 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@macisamuele macisamuele macisamuele approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.3.0
Development

Successfully merging this pull request may close these issues.

Nullability change in moshi JsonAdapter
2 participants
@cortinico @macisamuele