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.

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 last_affected example for clarity #174

Merged
merged 3 commits into from
Jul 14, 2023
Merged

Conversation

hayleycd
Copy link
Contributor

@hayleycd hayleycd commented Jul 6, 2023

Hopefully wraps up #150 and #146

View rendered example here.

Changes were also made to the affected.ranges.events fields to bring the formatting into line with the rest of the document. Fields were being rendered like this: "last_affected" where last_affected is preferred.

Copy link
Collaborator

@andrewpollock andrewpollock left a comment

Choose a reason for hiding this comment

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

LGTM, I'd like an extra reviewer to also give their opinion.

@andrewpollock
Copy link
Collaborator

Not sure if you've seen #35 (I hadn't until today, by happy coincidence). It has a lot of the original thinking behind the introduction of last_affected and I wonder if it's worth at a minimum referencing, or if there's any useful material to further enhance this PR?

@hayleycd
Copy link
Contributor Author

@andrewpollock I hadn't read through #35 before! It was interesting reading, but I'm a little wary of putting too much of that history in the schema. I feel like it may have the tendency to prompt more questions than it would answer and that section is already kind of long.

Copy link
Contributor

@oliverchang oliverchang left a comment

Choose a reason for hiding this comment

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

thanks! just some minor comments

docs/schema.md Outdated Show resolved Hide resolved
docs/schema.md Show resolved Hide resolved
docs/schema.md Outdated Show resolved Hide resolved
docs/schema.md Outdated Show resolved Hide resolved
Signed-off-by: Hayley Denbraver <[email protected]>
Signed-off-by: Hayley Denbraver <[email protected]>
Signed-off-by: Hayley Denbraver <[email protected]>
@hayleycd
Copy link
Contributor Author

Thanks @oliverchang ! I have addressed the comments. Let me know if there are any more changes.

@oliverchang oliverchang merged commit c32683a into ossf:main Jul 14, 2023
1 check passed
@Marcono1234
Copy link

Should #150 and #146 now be closed? At least for me (author of #150) I think the recent changes clarify the usage of last_affected.

@hayleycd
Copy link
Contributor Author

I agree.

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.

4 participants