Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add RecordEvent message for provenance #56

Merged
merged 3 commits into from
Mar 31, 2020

Conversation

skearnes
Copy link
Member

Fixes #44

Also changes id to record_id and makes it a string field for flexibility (people shouldn't be typing record IDs by hand anyway).

@skearnes skearnes requested a review from connorcoley March 31, 2020 00:14
Copy link
Collaborator

@connorcoley connorcoley left a comment

Choose a reason for hiding this comment

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

For uniformity, would it make sense to also make experiment_start used this new RecordEvent message?

We have both a Person and DateTime associated with the provenance:

Person experimenter = 1;
DateTime experiment_start = 3;

I don't necessarily believe this is a good idea, but I wanted to bring it up as something to consider if you hadn't already. I think I prefer the current solution of having the experimenter and start time being direct fields within ReactionProvenance.

@skearnes
Copy link
Member Author

For uniformity, would it make sense to also make experiment_start used this new RecordEvent message?

We have both a Person and DateTime associated with the provenance:

Person experimenter = 1;
DateTime experiment_start = 3;

I don't necessarily believe this is a good idea, but I wanted to bring it up as something to consider if you hadn't already. I think I prefer the current solution of having the experimenter and start time being direct fields within ReactionProvenance.

I considered this and also preferred the direct field approach.

@skearnes
Copy link
Member Author

Also, here's another link describing why required was discouraged for proto2: https://stackoverflow.com/a/31814967

@skearnes skearnes merged commit 641a120 into open-reaction-database:master Mar 31, 2020
@skearnes skearnes deleted the mod branch March 31, 2020 16:26
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provenance
2 participants