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

Make it possible to insert several items at once #363

Merged
merged 17 commits into from
Sep 24, 2024

Conversation

augustebaum
Copy link
Contributor

@augustebaum augustebaum commented Sep 19, 2024

  • Create put_several which is aliased to put
    • Only really put items once all have been converted to Items
  • Make the one-item put a special case of put_several
  • Make it a TypeError when Project.put receives a non-string key (closes Project keys must be strings #362)

@augustebaum augustebaum linked an issue Sep 19, 2024 that may be closed by this pull request
@augustebaum augustebaum changed the title 173 make it possible to insert several items at once Make it possible to insert several items at once Sep 19, 2024
@probabl-ai probabl-ai deleted a comment from github-actions bot Sep 19, 2024
Copy link
Contributor

github-actions bot commented Sep 19, 2024

Coverage Report for ./frontend

Status Category Percentage Covered / Total
🔵 Lines 86.62% 1062 / 1226
🔵 Statements 86.62% 1062 / 1226
🔵 Functions 58.92% 33 / 56
🔵 Branches 82.03% 105 / 128
File CoverageNo changed files found.
Generated in workflow #861

Copy link
Collaborator

@thomass-dev thomass-dev left a comment

Choose a reason for hiding this comment

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

Really happy to see singledispatchmethod in one of your PR <3.
Good idea !

src/skore/project.py Outdated Show resolved Hide resolved
src/skore/project.py Outdated Show resolved Hide resolved
src/skore/project.py Outdated Show resolved Hide resolved
@augustebaum augustebaum force-pushed the 173-make-it-possible-to-insert-several-items-at-once branch from a6b23c5 to aa2263c Compare September 19, 2024 15:30
@augustebaum augustebaum marked this pull request as draft September 20, 2024 10:06
@augustebaum augustebaum force-pushed the 173-make-it-possible-to-insert-several-items-at-once branch from aa2263c to 49bc9e1 Compare September 23, 2024 08:22
These are checks that were activated when we set
'requires-python = ">= 3.11"' in pyproject.toml
@augustebaum augustebaum marked this pull request as ready for review September 23, 2024 10:10
@augustebaum
Copy link
Contributor Author

Update to take into account design decisions in #173

src/skore/project.py Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
src/skore/project.py Outdated Show resolved Hide resolved
src/skore/project.py Outdated Show resolved Hide resolved
tests/unit/test_project.py Show resolved Hide resolved
- Raise a `ProjectPutError`
- When `on_error` is "raise", raise on the first error (rather than collecting all errors at the end).
Copy link
Collaborator

@thomass-dev thomass-dev left a comment

Choose a reason for hiding this comment

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

Nice work 👍🏻

@thomass-dev thomass-dev merged commit bac112d into main Sep 24, 2024
3 checks passed
@thomass-dev thomass-dev deleted the 173-make-it-possible-to-insert-several-items-at-once branch September 24, 2024 10:26
@augustebaum
Copy link
Contributor Author

In the end, we decided with @thomass-dev to simplify the behaviour:

  • The user can set on_error, which by default is "warn", both in put for single key-value pairs and for a dict
  • Whe the user sets on_error to "raise", we stop and raise upon the first exception found, rather than collecting all of them.

@thomass-dev
Copy link
Collaborator

A succession of put or one call to put_several have the same behaviour:

project.put("0", 0, on=error="warn")
project.put("1", 1, on=error="warn") 
project.put(2, 2, on=error="warn")  # bad
project.put("3", 3, on=error="warn")

# ==

project.put({"0": 0, "1": 1, 2: 2, "3": 3}, on=error="warn")

# OUTPUT
# ("0", 0), ("1", 1), ("3", 3) inserted
# warning on (2, 2)
project.put("0", 0, on=error="raise")
project.put("1", 1, on=error="raise") 
project.put(2, 2, on=error="raise")  # bad
project.put("3", 3, on=error="raise")

# ==

project.put({"0": 0, "1": 1, 2: 2, "3": 3}, on=error="raise")

# OUTPUT
# ("0", 0) and ("1", 1) inserted
# exception on (2, 2)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Project keys must be strings Make it possible to insert several items at once
4 participants