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

Remove escaping (\) of : in 2 identifiers regexes #290

Merged
merged 3 commits into from
Feb 24, 2025
Merged

Conversation

yarikoptic
Copy link
Member

@yarikoptic yarikoptic added the patch Increment the patch version when merged label Feb 24, 2025
Copy link
Member

@mvandenburgh mvandenburgh left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link

codecov bot commented Feb 24, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.88%. Comparing base (f0be592) to head (a7f23fe).
Report is 4 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #290   +/-   ##
=======================================
  Coverage   97.88%   97.88%           
=======================================
  Files          16       16           
  Lines        1983     1983           
=======================================
  Hits         1941     1941           
  Misses         42       42           
Flag Coverage Δ
unittests 97.88% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

We have them in:

    ❯ git grep 'DANDI\\*:' -- releases/0.6.9
    releases/0.6.9/dandiset.json:      "pattern": "^DANDI\\:\\d{6}$",
    releases/0.6.9/published-dandiset.json:      "pattern": "^DANDI:\\d{6}/\\d+\\.\\d+\\.\\d+",
    releases/0.6.9/published-dandiset.json:      "pattern": "^DANDI\\:\\d{6}$",
@yarikoptic
Copy link
Member Author

I believe we would need schema version boost since it is to be changed for

❯ git grep 'DANDI\\*:' -- releases/0.6.9
releases/0.6.9/dandiset.json:      "pattern": "^DANDI\\:\\d{6}$",
releases/0.6.9/published-dandiset.json:      "pattern": "^DANDI:\\d{6}/\\d+\\.\\d+\\.\\d+",
releases/0.6.9/published-dandiset.json:      "pattern": "^DANDI\\:\\d{6}$",

@yarikoptic
Copy link
Member Author

@mvandenburgh would it be possible to test adjusted/fixed up schema without releasing it so we could bundle more of potentially necessary fixes into the next (0.6.10?) release of the schema?

@candleindark tests fail with

_______________________________ test_migrate_044 _______________________________
dandischema/tests/test_metadata.py:432: in test_migrate_044
    newmeta_2 = migrate(newmeta, to_version=DANDI_SCHEMA_VERSION)
dandischema/metadata.py:435: in migrate
    _validate_obj_json(obj, _get_jsonschema_validator(obj_ver, "Dandiset"))
dandischema/metadata.py:231: in _get_jsonschema_validator
    r.raise_for_status()
.tox/py/lib/python3.12/site-packages/requests/models.py:1024: in raise_for_status
    raise HTTPError(http_error_msg, response=self)
E   requests.exceptions.HTTPError: 404 Client Error: Not Found for url: https://raw.githubusercontent.com/dandi/schema/master/releases/0.6.10/dandiset.json

I think it might relate to recent enhancements? in this case we must not check for current (proposed) version since it is not yet released.

@candleindark
Copy link
Member

@candleindark tests fail with

_______________________________ test_migrate_044 _______________________________
dandischema/tests/test_metadata.py:432: in test_migrate_044
    newmeta_2 = migrate(newmeta, to_version=DANDI_SCHEMA_VERSION)
dandischema/metadata.py:435: in migrate
    _validate_obj_json(obj, _get_jsonschema_validator(obj_ver, "Dandiset"))
dandischema/metadata.py:231: in _get_jsonschema_validator
    r.raise_for_status()
.tox/py/lib/python3.12/site-packages/requests/models.py:1024: in raise_for_status
    raise HTTPError(http_error_msg, response=self)
E   requests.exceptions.HTTPError: 404 Client Error: Not Found for url: https://raw.githubusercontent.com/dandi/schema/master/releases/0.6.10/dandiset.json

I think it might relate to recent enhancements? in this case we must not check for current (proposed) version since it is not yet released.

Let me take a look.

@mvandenburgh
Copy link
Member

That should be fine. I can build the schema locally and test it.

I'm currently dynamically modifying the schema to fix these regexes (dandi/dandi-archive@d57d9fb), so I can just leave that in until the new schema version is released.

@mvandenburgh
Copy link
Member

@yarikoptic @candleindark I think I found another issue with the current schema and ajv, so I think it's worth holding off on a release regardless.

@yarikoptic yarikoptic merged commit 782c421 into master Feb 24, 2025
64 checks passed
@yarikoptic yarikoptic deleted the bf-colonslash branch February 24, 2025 19:19
@mvandenburgh
Copy link
Member

@yarikoptic @candleindark I think I found another issue with the current schema and ajv, so I think it's worth holding off on a release regardless.

I described this issue in #286 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch Increment the patch version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove unnecessary backslash from regex patterns
3 participants