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

Clarify sentence about "last_affected" and "fixed" in docs. #310

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tiegz
Copy link

@tiegz tiegz commented Nov 7, 2024

From the docs:

Entries in the events array can contain either "last_affected" or "fixed" events, but not both.

This line could be referring to either of these events arrays:

{
  "events": [
    {"introduced": "0.0.0"},
    {"last_affected": "1.0.0", "fixed": "1.0.1"}
  ]
}
{
  "events": [
    {"introduced": "0.0.0"},
    {"last_affected": "1.0.0"},
    {"fixed": "1.0.1"}
  ]
}

The first one is invalid because it has an Object that contains two entries, but that is covered by this line from the docs:

Only a single type (either introduced, fixed, last_affected, limit) is allowed in each event object.

And according to this comment I think the latter invalid case is the one that is being referred to: #146 (comment)

So I suggest changing the wording to be more specific:

An events array can have entries containing either "last_affected" or "fixed" events, but not both.

@@ -830,8 +830,8 @@ Only **a single type** (either `introduced`, `fixed`, `last_affected`,
`limit`) is allowed in each event object. For instance,
`{"introduced": "1.0.0", "fixed": "1.0.2"}` is **invalid**.

Entries in the `events` array can contain either `last_affected` or `fixed`
events, but not both. It's **strongly recommended** to use `fixed` instead of
An events array can have entries containing either "last_affected" or "fixed" events,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, there's some semantic nuance here, which is a little tricky to convey, I think.

So to get all JSON-technical, the events array is an array of JSON (event) objects, and the intention of the statement under scrutiny is to refer to these event objects, as you've surmised.

https://github.com/ossf/osv-schema/blob/main/validation/schema.json#L92-L150

Suggested change
An events array can have entries containing either "last_affected" or "fixed" events,
Entries in the `events` array may be "last_affected" or "fixed" events,

Copy link
Author

Choose a reason for hiding this comment

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

@andrewpollock I think that it's possible that someone may still misread that suggestion as disallowing this case:

[..., {"last_affected": "1.0.0", "fixed": "1.0.1"}, ...]

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.

2 participants