oasis-tcs / csaf

OASIS CSAF TC: Supporting version control for Work Product artifacts developed by members of TC, including prose specifications and secondary artifacts like meeting minutes and productivity code
https://github.com/oasis-tcs/csaf
Other
151 stars 40 forks source link

Standardize on regex where requiring specific string matching #243

Closed santosomar closed 3 years ago

santosomar commented 3 years ago

This was an issue highlighted by dstrohl at:

https://github.com/oasis-open/csaf-documentation/issues/7

In the models section, always include the regular expression where matching is required. (sometimes you had "must be less than x characters long", sometimes you listed some of the items (like with the CVSS scores). It would be helpful to be able to simply copy the regex into systems instead of having to figure it out each time. (not a big deal, but if you are already there, it's a nice thing).

tschmidtb51 commented 3 years ago

@dstrohl: Can you please check whether we addressed your comment in the current Editor Draft for CSAF 2.0?

sthagen commented 3 years ago

I am not really in favor of using regular expressions like .+ instead of stating a minimum length ... essentially in cases when we can only restrain the dimension I would refrain from stating that as a pattern.

dstrohl commented 3 years ago

There's nothing to say you can't do both (so, say that the the max length was 100 characters or whatever, AND also have a validation pattern shown of ".{,100}"

Ultimately, my real point I guess though was to standardize on one approach, it's better to pick one approach, use regex, don't use regex, or use both everywhere than to sometimes use regex, and sometimes not... then developers end up having to figure out if this example is one that does not have regex, or if they just aren't seeing it, or whatever, adding friction to people taking it up.

To be fair, it's probably not needed though for things that are simply "somethings required here" and there is no real validation of the input needed other than if == "" then raise error.

Like I said also, it's not a major thing either, just trying to reduce friction for people.

dstrohl commented 3 years ago

@dstrohl: Can you please check whether we addressed your comment in the current Editor Draft for CSAF 2.0?

I scanned the editor draft and it looks good. (to be fair, I didn't read every case to validate consistency, but the ones I looked at looked good).

Dan

tschmidtb51 commented 3 years ago

@dstrohl: Can you please check whether we addressed your comment in the current Editor Draft for CSAF 2.0?

I scanned the editor draft and it looks good. (to be fair, I didn't read every case to validate consistency, but the ones I looked at looked good).

Dan

Thanks for checking.

tschmidtb51 commented 3 years ago

Ultimately, my real point I guess though was to standardize on one approach, it's better to pick one approach, use regex, don't use regex, or use both everywhere than to sometimes use regex, and sometimes not... then developers end up having to figure out if this example is one that does not have regex, or if they just aren't seeing it, or whatever, adding friction to people taking it up.

To be fair, it's probably not needed though for things that are simply "somethings required here" and there is no real validation of the input needed other than if == "" then raise error.

CSAF 2.0 prohibits empty strings, objects, arrays,... Therefore, we use the min{Length, Item, Properties} a lot. If there is a specific format required we use regex. As tools should always use the JSON schema to validate CSAF documents I guess the problem is solved.

As a side note: IMHO it makes sense to not use regex for trivial things like minLength for a few reasons:

sthagen commented 3 years ago

I suggest to close without action given that the reporter scanned the editor draft and was OK with the new use of constraints.

santosomar commented 3 years ago

I agree. I also consider this closed.