Skip to content

Removes merge field from ReviewEvent and regenerates files#74

Merged
se7entyse7en merged 1 commit intosrc-d:masterfrom
se7entyse7en:merge-field-removal
Feb 21, 2019
Merged

Removes merge field from ReviewEvent and regenerates files#74
se7entyse7en merged 1 commit intosrc-d:masterfrom
se7entyse7en:merge-field-removal

Conversation

@se7entyse7en
Copy link
Copy Markdown
Contributor

@se7entyse7en se7entyse7en commented Feb 14, 2019

First task from issue: src-d/lookout#83.

@carlosms
Copy link
Copy Markdown
Contributor

Looks like the code generated by you and by travis is different. Maybe there is a problem with the versions used.

Signed-off-by: Lou Marvin Caraig <loumarvincaraig@gmail.com>
@se7entyse7en
Copy link
Copy Markdown
Contributor Author

@carlosms you're right, the version was wrong. Thanks!

@carlosms
Copy link
Copy Markdown
Contributor

Maybe we should make the field reserved?

@se7entyse7en
Copy link
Copy Markdown
Contributor Author

@carlosms I don't completely understand what is the case that would cause problems. AFAIU the problem arises if now we remove it and then let's say next week we add another field with the same name and number, but for example a different type. But that could be a problem if that object is interpreted using the old version of the proto file right?

@smacker
Copy link
Copy Markdown
Contributor

smacker commented Feb 14, 2019

imo we are still at the very early stage of lookout and pollute it with reserved doesn't make much sense to me.

@carlosms
Copy link
Copy Markdown
Contributor

🤷‍♂️ ok let's skip the reserved

@se7entyse7en se7entyse7en merged commit 5b98f1e into src-d:master Feb 21, 2019
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.

3 participants