Skip to content

Update pattern for FullyQualifiedReference in schema#220

Open
FriedrichBu wants to merge 5 commits intobitol-io:devfrom
FriedrichBu:patch-1
Open

Update pattern for FullyQualifiedReference in schema#220
FriedrichBu wants to merge 5 commits intobitol-io:devfrom
FriedrichBu:patch-1

Conversation

@FriedrichBu
Copy link
Copy Markdown

YAML-Files not only have the filename extension .yaml, but also .yml. Thus, it'd make sense to include .yml as valid filename extention as well in the reference schema.

jgperrin and others added 3 commits December 8, 2025 06:57
This is it... release to v3.1.0
YAML-Files not only have the filename extension .yaml, but also .yml. Thus, it'd make sense to include .yml as valid filename extenstion in the reference schema
@FriedrichBu FriedrichBu requested a review from a team as a code owner December 17, 2025 11:58
@pflooky pflooky added the bug Something isn't working label Dec 18, 2025
@pflooky pflooky added this to the ODCS v3.1.1 milestone Dec 18, 2025
@pflooky
Copy link
Copy Markdown
Contributor

pflooky commented Dec 18, 2025

Hey @FriedrichBu , thanks for raising the PR. Looks like we could put this into a patch release of ODCS. Can you make the PR to dev branch instead?

@pflooky
Copy link
Copy Markdown
Contributor

pflooky commented Dec 18, 2025

This also perhaps raises another question about whether we want to restrict file name extensions to also include .odcs.yaml or .odcs.yml. We have these two extensions defined within schema store as seen here: https://github.com/SchemaStore/schemastore/blob/master/src/api/json/catalog.json#L4487

Copy link
Copy Markdown
Contributor

@jgperrin jgperrin left a comment

Choose a reason for hiding this comment

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

I am definitely not the person to review regex ;)

@dccakes
Copy link
Copy Markdown
Contributor

dccakes commented Dec 18, 2025

Agree on adding yml | yaml to the schema as a patch.

@FriedrichBu FriedrichBu changed the base branch from main to dev December 19, 2025 14:23
@FriedrichBu
Copy link
Copy Markdown
Author

FriedrichBu commented Dec 19, 2025

Hi @pflooky,
thanks for the positive feedback. I have changed the PR to the dev branch.

`\\.ya?ml#` is cleaner & shorter than `\\.(?:yaml|yml)#`
`\\.ya?ml#` is cleaner & shorter than `\\.(?:yaml|yml)#`
@FriedrichBu
Copy link
Copy Markdown
Author

@pflooky, I have been thinking about ocds.yml and ocds.yaml.

We can add (?:\.ocds)? (equals an optional .ocds) before \.ya?ml. However, in the string part before \.ya?ml anyways . and A-Za-z are allowed ([A-Za-z0-9._\\-\\/]), which already allows .ocds, but also others like .extra.yaml or .other.yml.
I am not sure, but I think it would be necessary to disallow any further . in the base filename apart from those in .ocds and .yaml. Is that desirable?

If yes, a bigger change would be required to split the variants that are possible before .yml and .yaml:

  • Same folder as current contract: data-contract-v1.yaml#/schema
  • Full path: file:///path/to/data-contract-v1.yaml#/schema
  • URL: https://example.com/data-contract-v1.yaml#/schema
  • Relative path: ../../path/to/data-contract-v1.yaml#/schema

to enable . only in case of URL for subdomain.domain.

@FriedrichBu
Copy link
Copy Markdown
Author

I further noticed that from external references:

file:///path/to/data-contract-v1.yaml

is currently not allowed by the pattern.
(?:https?:\\/\\/)? should be extended to (?:https?:\\/\\/|file:\\/\\/\\/)? to enable file:///

So in total: "^(?:(?:https?:\\/\\/|file:\\/\\/\\/)?[A-Za-z0-9._\\-\\/]+(?:\\.ocds)?\\.ya?ml#)?\\/?[A-Za-z_][A-Za-z0-9_]*\\/[A-Za-z0-9_-]+(?:\\/[A-Za-z_][A-Za-z0-9_]*\\/[A-Za-z0-9_-]+)*$

With distunigshing .ocds as suggested as option above: ^(?:(?:https?:\\/\\/[A-Za-z0-9._\\-]+(?::\d+)?|file:\\/\\/\\/|(?:\.{1,2}[\\/])+)?(?:[A-Za-z0-9._\\-]+[\\/])*?([A-Za-z0-9_\\-]+)?(?:\\.ocds)?\\.ya?ml#)?\\/?[A-Za-z_][A-Za-z0-9_]*\\/[A-Za-z0-9_-]+(?:\\/[A-Za-z_][A-Za-z0-9_]*\\/[A-Za-z0-9_-]+)*$

@jgperrin
Copy link
Copy Markdown
Contributor

Can we merge or close this one?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants