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

feat(path)!: consolidated path libraries #334

Merged
merged 14 commits into from
Oct 6, 2023
Merged

feat(path)!: consolidated path libraries #334

merged 14 commits into from
Oct 6, 2023

Conversation

hacdias
Copy link
Member

@hacdias hacdias commented Jun 6, 2023

This is the long-awaited consolidation of the multiple path libraries we've had, which closes #198. For more information, see the changelog in the PR.

Kubo PR: ipfs/kubo#10063.

@codecov
Copy link

codecov bot commented Jun 6, 2023

Codecov Report

Merging #334 (9252a2b) into main (b388690) will increase coverage by 0.03%.
The diff coverage is 71.54%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #334      +/-   ##
==========================================
+ Coverage   65.69%   65.72%   +0.03%     
==========================================
  Files         209      207       -2     
  Lines       25196    25131      -65     
==========================================
- Hits        16552    16518      -34     
+ Misses       7177     7151      -26     
+ Partials     1467     1462       -5     
Files Coverage Δ
gateway/assets/assets.go 69.62% <100.00%> (ø)
gateway/errors.go 86.48% <100.00%> (-0.36%) ⬇️
gateway/gateway.go 85.60% <ø> (+3.71%) ⬆️
gateway/handler_block.go 69.76% <100.00%> (ø)
gateway/handler_car.go 75.42% <100.00%> (ø)
gateway/handler_defaults.go 31.76% <100.00%> (ø)
gateway/handler_tar.go 79.36% <100.00%> (ø)
gateway/handler_unixfs_file.go 73.01% <100.00%> (ø)
namesys/base.go 88.60% <ø> (ø)
namesys/republisher/repub.go 57.84% <100.00%> (ø)
... and 19 more

... and 12 files with indirect coverage changes

@hacdias hacdias changed the title refactor(path): consolidate path libraries feat(path): consolidated path libraries Jun 7, 2023
@hacdias hacdias force-pushed the issue/198 branch 2 times, most recently from 317e218 to c280652 Compare August 10, 2023 15:06
@hacdias
Copy link
Member Author

hacdias commented Aug 10, 2023

@ipfs/kubo-maintainers I think this is something important to get out of the door for many reasons (see #198). I just rebased this work I did in June on top of the recent changes. I can only imagine this having large repercussions thorough our other packages, especially Kubo. Therefore, before continuing, I would like to ask for some feedback regarding the Path interface.

One of the biggest changes is the removal of the IsValid() bool function from the path type. The idea is that if a path exists, then it must be valid. There's no point on passing around a path that will be invalid and always calling for IsValid(). Now the verification is done at creation time. This is what scares me the most when bubbling this to Kubo, as processes that would otherwise fail later in the process will now fail earlier. Or maybe not the case.

@hacdias hacdias changed the title feat(path): consolidated path libraries feat(path)!: consolidated path libraries Aug 10, 2023
@hacdias hacdias requested a review from a team August 10, 2023 15:38
Copy link
Contributor

@Jorropo Jorropo left a comment

Choose a reason for hiding this comment

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

I shouldn't look at the implementation before the API so the code review is half done.

I don't understand what is the difference between ResolvedPath and ImmutablePath ?

path/path.go Show resolved Hide resolved
path/path.go Outdated Show resolved Hide resolved
path/path.go Show resolved Hide resolved
path/path.go Outdated Show resolved Hide resolved
path/path.go Outdated Show resolved Hide resolved
path/path.go Outdated Show resolved Hide resolved
path/path.go Outdated Show resolved Hide resolved
path/path.go Outdated Show resolved Hide resolved
path/path.go Show resolved Hide resolved
path/path.go Outdated Show resolved Hide resolved
Copy link
Contributor

@aschmahmann aschmahmann left a comment

Choose a reason for hiding this comment

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

Thank you for starting this 🙏. The current state of the path libraries is .... too much

path/path.go Outdated Show resolved Hide resolved
path/path.go Outdated Show resolved Hide resolved
path/path.go Outdated Show resolved Hide resolved
path/path.go Outdated Show resolved Hide resolved
path/path.go Outdated Show resolved Hide resolved
path/path.go Outdated Show resolved Hide resolved
path/path.go Outdated Show resolved Hide resolved
path/path.go Outdated Show resolved Hide resolved
path/path.go Outdated Show resolved Hide resolved
path/path.go Outdated Show resolved Hide resolved
@hacdias hacdias force-pushed the issue/198 branch 7 times, most recently from c912f0c to bea6cf9 Compare August 22, 2023 12:52
@hacdias
Copy link
Member Author

hacdias commented Sep 25, 2023

@aschmahmann @lidel I want to ping you for a re-review. I've already addressed the feedback, tests are passing here and in Kubo. This PR is almost turning 4 months old and I really don't want that to happen.

lidel
lidel previously requested changes Oct 5, 2023
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

Some feedback inline, mostly about making interface more clear, not tied to our namespaces (allowing people to use boxo with /foo without having to fork) and avoiding cognitive noise caused by legacy decisions in Kubo.

To avoid making waves this difficult month, I think we should minimize risk and ship this in Kubo 0.24, and we should wait with merge until 0.23 ships.

path/path.go Outdated Show resolved Hide resolved
path/resolver/resolver.go Outdated Show resolved Hide resolved
path/path.go Outdated Show resolved Hide resolved
path/path.go Outdated Show resolved Hide resolved
path/path.go Outdated Show resolved Hide resolved
path/path.go Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
gateway/gateway_test.go Outdated Show resolved Hide resolved
path/path.go Outdated Show resolved Hide resolved
@hacdias
Copy link
Member Author

hacdias commented Oct 5, 2023

To avoid making waves this difficult month, I think we should minimize risk and ship this in Kubo 0.24, and we should wait with merge until 0.23 ships.

It already has: https:/ipfs/kubo/releases/tag/v0.23.0!

@hacdias hacdias requested a review from lidel October 5, 2023 17:55
@hacdias hacdias dismissed stale reviews from Jorropo and lidel October 6, 2023 10:55

addressed

@hacdias
Copy link
Member Author

hacdias commented Oct 6, 2023

I am going to go ahead and merge this. I think it has marinated for enough time and I've already addressed the feedbacks we had. Tests are passing here and in Kubo, and Kubo 0.23 has been released. So merging this at the beginning of the new release cycle (0.24) will allow us to test it until the final release of 0.24.

@hacdias hacdias merged commit 85c180e into main Oct 6, 2023
13 of 14 checks passed
@hacdias hacdias deleted the issue/198 branch October 6, 2023 14:04
@BigLep BigLep mentioned this pull request Nov 9, 2023
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Consolidate IPFS Path libraries under boxo/path
4 participants