Skip to content

Week of Apr 20#455

Merged
duglin merged 4 commits into
xregistry:mainfrom
duglin:Apr20
Apr 23, 2026
Merged

Week of Apr 20#455
duglin merged 4 commits into
xregistry:mainfrom
duglin:Apr20

Conversation

@duglin

@duglin duglin commented Apr 20, 2026

Copy link
Copy Markdown
Contributor
  • typos, wording tweaks and tooling updates
  • split formatvalidated and compatvalidated - e.g:
"formatvalidated": "false, reason we didn't check"

becomes:

"formatvalidated": false,
"formatvalidatedreason": "reason we didn't check"

- typos
- split formatvalidated/compatvalidated into bool & string
- s/details/reason/g

Signed-off-by: Doug Davis <duglin@gmail.com>

@jskeet jskeet left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hope these are useful. Some are definitely matters of opinion (performed vs attempted) but a few are genuinely mistakes...

Comment thread core/spec.md Outdated
Additionally, when `"false"`, if `compatibility` validation is enabled,
the Version's
When `false`, the
[`formatvalidatedreason`](#formatvalidatedreason-attribute) MUST attribute

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
[`formatvalidatedreason`](#formatvalidatedreason-attribute) MUST attribute
[`formatvalidatedreason`](#formatvalidatedreason-attribute) attribute

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed

Comment thread core/spec.md Outdated
When `false`, the
[`formatvalidatedreason`](#formatvalidatedreason-attribute) MUST attribute
MUST be present with an explanation as to why the server was unable to
attempt validation of the Version. Additionally, when `false`, if

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
attempt validation of the Version. Additionally, when `false`, if
perform validation of the Version. Additionally, when `false`, if

(or just "validate"). I suspect in every case it is attempting validation, but the reason is why it can't actually perform the validation.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

(As it happens, looking down to line 3300, it seems I coincidentally stumbled on the same terminology...)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I can live with "perform" as long as people don't read it as "did validation and failed".
To be clear, there are 3 states:

  • didn't even try to check
  • checked and failed
  • checked and passed

These sentences are trying to talk about the first one.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Right, but I think I'd categorize it as "tried to check, but couldn't even do the check due to preconditions, let alone give you a result". Is that correct, or is it genuinely "I made no attempt to even see whether I could check"?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't think we draw a distinction between those two. It's a boolean thing: either we check or we don't. Whether we "don't check" because of a missing precondition or because the feature as been turned off (for the most part) doesn't really matter. The only diff is that when the feature is turned on (and strict=false) we will at least tell the user why we didn't "attempt" to check. We do this because when they have the feature enabled but the server didn't even try, the user might have a hard time following the breadcrumbs to figure out why the server didn't attempt it.

We could technically make the "reason" be present with "feature turned off" even in the "feature disabled" cases, but we're trying to avoid including flags related to disable features - to keep things from being annoyingly verbose.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Maybe it would be simplest to remove "attempt" entirely then - just "didn't check" seems simpler than "didn't try to check".

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That would be nice, but let me ask this... if a newbie reads:

the server did not validate the entity

What are the odds of someone reading that as "validation failed" ? I ask because often when someone says something like "I did not confirm that your statement is true", I think it's ambiguous as to whether they mean "I tried to confirm it but I failed to find the proof that it's true", or "I did not even try to find the proof". See my concern?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Yup, but I think "perform validation" is then simpler than "attempt to validate". Basically the word "attempt" feels like there's more intent to it than simply "either we check or we don't".

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ok check latest commit to see if it looks ok

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Yup, much better, thanks.

Comment thread core/spec.md Outdated
value of `false`, this attribute MUST provide information as to why the
format validation check was not performed.

Due to this attribute being server managed, and influenced by the state of

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This feels like it lacks a bit of context - remain unchanged under what circumstances?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

per the text "as its (this attribute's) value changes".

Would this read better to you:

Normally, as attribute values change, the entity itself is marked as changed (e.g. the modifiedat and epoch attributes are updated). However, since this attribute is server managed and can change due to other Versions being modified, similar to isdefault, changes to this attribute MUST NOT be treated as updates to the entity.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I see what you're getting at, but "changes to this attribute" still sounds like something is trying to change the attribute value.

Not sure of a better wording at the moment; I wonder whether some centralized notion of "computed, output-only" would be useful.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

hmm I kind of like that idea. does this work for you:

  • Unless otherwise stated, server managed changes to read-only attributes
    MUST NOT be treated the same as user initiated changes to non-read-only
    attributes with respect to updating attributes such as modifiedat and
    epoch. In other words, as these read-only attributes are changed, the
    entity MUST NOT be considered "updated".

Thinking about adding it to the generic Attribute and Extensions section

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Right - that sounds great to me.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ok done - and I removed all spots (including the one for "isdefault") that said the same thing.
I did add a link to this new text though.

Comment thread core/spec.md Outdated
- MUST be present when [`formatvalidated`](#formatvalidated-attribute) is
present with a value of `false`.
- MUST NOT be present when [`formatvalidated`](#formatvalidated-attribute) is
absent.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
absent.
absent or has a value of `true`.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

good catch!

Comment thread core/spec.md Outdated
is enabled, this attribute will indicate whether or not the server has
validated that the Version conforms to the rules defined by its Resource's
`meta.compatibility` attribute's value.
attempted to validate that the Version conforms to the rules defined by its

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think it would be better to use the same wording as for formatvalidated here. (Although if formatvalidated is false, it really hasn't even attempted compatibility validation, let alone actually performed validation...)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Can you elaborate? The wording already looks similar to me

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Your 2nd sentence about "attempted" vs "performed" makes me wonder about your other request to use "perform" instead of "attempted".

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I'm now not quite sure what I was thinking of at the time, but I think we can probably discuss this together with the whole attempted/performed aspect.

Comment thread core/spec.md Outdated

A value that starts with `false` indicates that the server did not attempt
to validated the Version. This can happen when
A value of `false` indicates that the server did not attempt to validated

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
A value of `false` indicates that the server did not attempt to validated
A value of `false` indicates that the server did not attempt to validate

(And separately, as elsewhere, the "attempt" part doesn't feel quite right to me. Happy to be overruled on everything attempt-based though.)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

Comment thread core/spec.md Outdated
When `"false"`, this value MUST include some additional text (the `<REASON>`)
indicating why the server was unable to attempt validation of the Version.
When `false`, the
[`formatvalidatedreason`](#formatvalidatedreason-attribute) MUST attribute

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
[`formatvalidatedreason`](#formatvalidatedreason-attribute) MUST attribute
[`compatibilityvalidatedreason`](#compatibilityvalidatedreason-attribute) attribute

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

doi

Comment thread core/spec.md Outdated
present with a value of `false`.
- MUST NOT be present when
[`compatibilityvalidated`](#compatibilityvalidated-attribute) is
absent.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
absent.
absent or has a value of `true`.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yup!

Comment thread core/spec.md Outdated
- Constraints:
- OPTIONAL
- When specified, MUST be a non-empty string.
- When specified, MUST be either `true` or `false`, case-sensitive.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
- When specified, MUST be either `true` or `false`, case-sensitive.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

good catch - it's supposed to be in the previous section. Moved it.

Comment thread core/spec.md Outdated
- Description: When
[`format` validation](./model.md#groupsstringresourcesstringvalidateformat)
is enabled, this attribute will indicate whether or not the server has
attempted to validated that the Version conforms to the rules defined by

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Just spotted this...

Suggested change
attempted to validated that the Version conforms to the rules defined by
attempted to validate that the Version conforms to the rules defined by

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

Comment thread core/spec.md Outdated
is enabled, this attribute will indicate whether or not the server has
validated that the Version conforms to the rules defined by its Resource's
`meta.compatibility` attribute's value.
attempted to validate that the Version conforms to the rules defined by its

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I'm now not quite sure what I was thinking of at the time, but I think we can probably discuss this together with the whole attempted/performed aspect.

duglin added 3 commits April 23, 2026 12:44
Signed-off-by: Doug Davis <duglin@gmail.com>
Signed-off-by: Doug Davis <duglin@gmail.com>
Signed-off-by: Doug Davis <duglin@gmail.com>
@duglin

duglin commented Apr 23, 2026

Copy link
Copy Markdown
Contributor Author

Approved on the 4/23 call

@duglin
duglin merged commit a8b5953 into xregistry:main Apr 23, 2026
2 checks passed
@duglin
duglin deleted the Apr20 branch May 4, 2026 16:19
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