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

Update for Pydantic v2 #1381

Merged
merged 5 commits into from
Feb 23, 2024
Merged

Update for Pydantic v2 #1381

merged 5 commits into from
Feb 23, 2024

Conversation

jwodder
Copy link
Member

@jwodder jwodder commented Dec 18, 2023

Closes #1352.

Blocked by dandi/dandi-archive#1820.

Do not merge until after dandi/dandi-schema#203 is merged & released. Once that is done, the "[TEMP] Use Pydantic 2.0 branch of dandischema" commit in this PR should be replaced with a commit that sets the dandischema version requirement to ~= 0.9.0 instead. [Done]

@jwodder jwodder added minor Increment the minor version when merged dependencies Update one or more dependencies version blocked Blocked by some needed development/fix labels Dec 18, 2023
Copy link

codecov bot commented Dec 18, 2023

Codecov Report

Attention: Patch coverage is 87.14286% with 9 lines in your changes are missing coverage. Please review.

Project coverage is 88.57%. Comparing base (7ca670b) to head (728882f).

Files Patch % Lines
dandi/utils.py 81.81% 4 Missing ⚠️
dandi/dandiarchive.py 33.33% 2 Missing ⚠️
dandi/cli/cmd_download.py 66.66% 1 Missing ⚠️
dandi/dandiapi.py 95.83% 1 Missing ⚠️
dandi/files/bases.py 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1381      +/-   ##
==========================================
+ Coverage   88.49%   88.57%   +0.07%     
==========================================
  Files          77       77              
  Lines       10492    10493       +1     
==========================================
+ Hits         9285     9294       +9     
+ Misses       1207     1199       -8     
Flag Coverage Δ
unittests 88.57% <87.14%> (+0.07%) ⬆️

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.

Copy link
Member

@yarikoptic yarikoptic left a comment

Choose a reason for hiding this comment

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

Looks LGTM to me.

Copy link
Contributor

@candleindark candleindark left a comment

Choose a reason for hiding this comment

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

LGTM

@CodyCBakerPhD
Copy link
Contributor

@yarikoptic What was the timeline on getting this merged/released?

@yarikoptic
Copy link
Member

Originally planned right after coming back from holidays, but that fell through.
I just followed up on dandi/dandi-schema#203 (comment) -- we should first ensure that there is no metadata migrations to be done, then we would merge that one and start going through the depends like here. But overall -- I hope that by the end of this week we get moving with that.

@jwodder jwodder removed the blocked Blocked by some needed development/fix label Jan 22, 2024
@jwodder
Copy link
Member Author

jwodder commented Jan 22, 2024

@yarikoptic Various tests are currently failing because the Archive doesn't yet accept schema version 0.6.5.

@yarikoptic
Copy link
Member

@jwodder - could we TEMP test against version in

@jwodder
Copy link
Member Author

jwodder commented Feb 5, 2024

@yarikoptic Testing in dandi-cli requires a Docker image of the Archive PR. It would be easier to instead add a temporary commit in the Archive PR for running the CLI integration tests against this PR.

@jwodder jwodder marked this pull request as ready for review February 21, 2024 19:44
@jwodder jwodder removed the blocked Blocked by some needed development/fix label Feb 21, 2024

If ``path`` is already an absolute URL, it is returned unchanged.
"""
if path.lower().startswith(("http://", "https://")):
Copy link
Member

Choose a reason for hiding this comment

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

Function name and description suggests being applicable to URL of any transport, not just https so better be smth like

Suggested change
if path.lower().startswith(("http://", "https://")):
if re.match('[a-z]+://', path.lower()):

to avoid surprises.

Copy link
Member

Choose a reason for hiding this comment

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

but also how it is different from a standard urllib's join?

In [3]: urllib.parse.urljoin('http://a.c/', '/1/2/3')
Out[3]: 'http://a.c/1/2/3'

In [4]: urllib.parse.urljoin('http://a.c/', 's3://1/2/3')
Out[4]: 's3://1/2/3'

Copy link
Member Author

Choose a reason for hiding this comment

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

  • The function is only meant for HTTP(S) URLs.

  • It differs from urljoin() for URLs that have a path that doesn't end in /:

    >>> urljoin("https://example.com/foo/bar", "quux")
    'https://example.com/foo/quux'
    

    joinurl() would instead give https://example.com/foo/bar/quux.

Copy link
Member

Choose a reason for hiding this comment

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

  • then better name it httpurljoin or at least describe it in its description
  • also please add into description this difference in behavior as rationale

Copy link
Member Author

Choose a reason for hiding this comment

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

Done: 728882f

@yarikoptic
Copy link
Member

thank you @jwodder ! let's proceed but remember to not release until archive is released (attn @waxlamp - we need to sync release process etc )

@yarikoptic yarikoptic merged commit aa55b89 into master Feb 23, 2024
27 of 28 checks passed
@yarikoptic yarikoptic deleted the gh-1352 branch February 23, 2024 19:01
Copy link

🚀 PR was released in 0.60.0 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Update one or more dependencies version minor Increment the minor version when merged released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update for Pydantic 2.0
4 participants