GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — refactor!: Make KeyValuePairs an alias for BTreeMap by nightkr · Pull Request #889 · stackabletech/operator-rs

Conversation

nightkr commented Oct 14, 2024 •

edited by Techassi

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

This is effectively an extended version of #888 (and supersedes it).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

nightkr added 5 commits

October 11, 2024 15:02


          Replace KeyValuePairs' internal BTreeSet with BTreeMap

7d62fc7

Fixes #887


          Rename PairAlreadyExists => KeyAlreadyExists

f102e4d


          Reorganize kvp so that KeyValuePairs is an alias for BTreeMap instead…

f7ac175

… of newtyping it


          Changelog

b215e8e


          Drop unused delegate dependency

41ce00e

nightkr requested a review from Techassi

October 14, 2024 09:27

nightkr linked an issue

Oct 14, 2024

that may be closed by this pull request

KeyValuePairs::extend implements a nonsense merge #887

Closed

Techassi requested changes

Oct 16, 2024

View reviewed changes

Techassi left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Very nice job, I like it!

Left a bunch of small comments, nothing major.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

crates/stackable-operator/src/kvp/annotation/mod.rs Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

crates/stackable-operator/src/kvp/key.rs Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

crates/stackable-operator/src/kvp/label/mod.rs Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

crates/stackable-operator/src/kvp/mod.rs Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

crates/stackable-operator/src/kvp/mod.rs Outdated

Comment on lines +24 to +27

		#[cfg(doc)]
		use k8s_openapi::apimachinery::pkg::apis::meta::v1::ObjectMeta;
		#[cfg(doc)]
		use std::ops::Deref;

Techassi Oct 16, 2024

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

suggestion: Move these imports to the std library and external crate imports at the top of the file.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

nightkr Oct 16, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I think it makes sense to call out doc-only "fake" imports as different from regular one (just like publics), though we definitely haven't done a great job at consistency around that in general.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

crates/stackable-operator/src/kvp/mod.rs Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

crates/stackable-operator/src/kvp/mod.rs Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

crates/stackable-operator/src/kvp/mod.rs Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

crates/stackable-operator/CHANGELOG.md Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

crates/stackable-operator/CHANGELOG.md Resolved

Uh oh!

There was an error while loading. Please reload this page.

Techassi mentioned this pull request

Oct 16, 2024

Replace KeyValuePairs' internal BTreeSet with BTreeMap #888

Merged

nightkr and others added 10 commits

October 16, 2024 15:08


          Use typed Key in errors

1e4cffe


          Docs for label::well_known::vendor_stackable

a6101a4


          Separate public modules

e21a8f5


          Rename Value generics from T -> V

17458e1


          Clean up tests

8f6daa0


          Rename label::sets to label::well_known::sets

464fdda


          Merge branch 'main' into feature/kvp-reorg

5fd721a


          Oops

98bdabf


          chore: Cleanup

aba6663


          chore: Merge branch 'main' into feature/kvp-reorg

836e633

Techassi changed the title ~~Reorganize kvp so that KeyValuePairs is an alias for BTreeMap instead of newtyping it~~ refactor!: Make KeyValuePairs an alias for BTreeMap

Aug 12, 2025

This branch has not been deployed

No deployments

Reviewers

Techassi

Techassi requested changes

Requested changes must be addressed to merge this pull request.

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

KeyValuePairs::extend implements a nonsense merge

Uh oh!

There was an error while loading. Please reload this page.

2 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

nightkr commented Oct 14, 2024 • edited by Techassi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Techassi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Techassi Oct 16, 2024

Choose a reason for hiding this comment

Uh oh!

nightkr Oct 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

nightkr commented Oct 14, 2024 •

edited by Techassi

Loading

Uh oh!

There was an error while loading. Please reload this page.

nightkr Oct 16, 2024 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.