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.

Sign up for GitHub

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

Jump to bottom

Add once_map package #1665

Open
wants to merge 1 commit into
base: spr/main/3ba8d8f0
Choose a base branch
from
Open

Add once_map package #1665

wants to merge 1 commit into from

Conversation

maciektr
Copy link
Contributor

@maciektr maciektr commented Oct 22, 2024 •
edited
Loading

Stack:

⚠️ Part of a stack created by spr. Do not merge manually using the UI - doing so may have unexpected results.

This was referenced Oct 22, 2024
Open
Open
@maciektr maciektr force-pushed the spr/main/293590d9 branch 2 times, most recently from a4d0ee0 to 3fc352c Compare October 26, 2024 20:38
@maciektr maciektr force-pushed the spr/main/293590d9 branch 4 times, most recently from 31cbe81 to 33a0dad Compare November 4, 2024 22:52
@maciektr maciektr force-pushed the spr/main/293590d9 branch 2 times, most recently from aa612cb to c117830 Compare November 19, 2024 21:01
@maciektr
Add once_map package
a1502d4 
commit-id:293590d9
@maciektr maciektr marked this pull request as ready for review November 25, 2024 14:42
@maciektr maciektr requested a review from a team as a code owner November 25, 2024 14:42
mkaput
mkaput reviewed Nov 25, 2024
View reviewed changes
utils/once-map/src/lib.rs
///
/// Note that this always clones the value out of the underlying map. Because
/// of this, it's common to wrap the `V` in an `Arc<V>` to make cloning cheap.
pub struct OnceMap<K, V, H = RandomState> {
Copy link
Member

Choose a reason for hiding this comment

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

this is basically dataloader, isn't it?

wawel37
wawel37 approved these changes Nov 26, 2024
View reviewed changes
Copy link
Member

@wawel37 wawel37 left a comment

Choose a reason for hiding this comment

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

I don't see OnceMap tests in the original repo, but it seems that some bugs appeared there not that long ago (astral-sh/uv#3987). I am not saying that it's unstable or anything, but i think it would be nice to have some tests for it in the future.

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

@wawel37 wawel37 wawel37 approved these changes

@mkaput mkaput mkaput approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@maciektr @mkaput @wawel37