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

irmin: raise exception on duplicate Conf key name #2252

Merged
merged 3 commits into from
May 24, 2023

Conversation

metanivek
Copy link
Member

This bit me while working on the memory-bounded LRU (no, I obviously did not copy-pasta an existing key 🫣).

I don't think there is a valid use case for calling key twice with the same name but different types, and it can result in annoying to debug runtime behavior since it will silently return the default value for the overwritten key.

src/irmin/conf.ml Outdated Show resolved Hide resolved
@metanivek metanivek requested review from art-w and adatario May 18, 2023 15:32
Copy link
Contributor

@adatario adatario left a comment

Choose a reason for hiding this comment

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

Small comment on usage of if-then-else vs. match otherwise lgtm!

src/irmin/conf.ml Outdated Show resolved Hide resolved
@metanivek metanivek added the no-changelog-needed No changelog is needed here label May 19, 2023
@metanivek metanivek requested a review from adatario May 22, 2023 17:51
Copy link
Contributor

@adatario adatario left a comment

Choose a reason for hiding this comment

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

lgtm!

@metanivek metanivek force-pushed the conf/err_on_dupe_keys branch 2 times, most recently from 3fefcf6 to 65afbc6 Compare May 23, 2023 18:35
@metanivek metanivek removed the no-changelog-needed No changelog is needed here label May 23, 2023
@metanivek metanivek merged commit 0258c37 into mirage:main May 24, 2023
4 of 5 checks passed
@metanivek metanivek deleted the conf/err_on_dupe_keys branch May 24, 2023 16:14
metanivek added a commit to metanivek/opam-repository that referenced this pull request Jul 4, 2023
…min-pack, irmin-pack-tools, irmin-mirage, irmin-mirage-graphql, irmin-mirage-git, irmin-http, irmin-graphql, irmin-git, irmin-fs, irmin-containers, irmin-cli, irmin-chunk and irmin-bench (3.8.0)

CHANGES:

### Added

- **irmin**
  - Change behavior of `Irmin.Conf.key` to disallow duplicate key names by
    default. Add `allow_duplicate` optional argument to override. (mirage/irmin#2252,
    @metanivek)

- **irmin-pack**
  - Add maximum memory as an alternative configuration option, `lru_max_memory`,
    for setting LRU capacity. (@metanivek, mirage/irmin#2254)

### Changed

- **irmin**
  - Lower bound for `mtime` is now `2.0.0` (mirage/irmin#2166, @patricoferris)

- **irmin-mirage-git**
  - Lower bound for `mirage-kv` is now `6.0.0` (mirage/irmin#2256, @metanivek)

### Fixed

- **irmin-cli**
  - Changed `--store irf` to `--store fs` to align the CLI with what is
    published on the Irmin website (mirage/irmin#2243, @wyn)
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.

None yet

2 participants