Added `DefaultBtreeMap` and a `set_default` method. #10

rljacobson · 2020-08-30T18:40:38Z

Added DefaultBtreeMap and a set_default method to both HashMap and DefaultBTreeMap.

codecov-commenter · 2020-08-30T18:44:32Z

Codecov Report

Merging #10 into master will decrease coverage by 2.14%.
The diff coverage is 96.66%.

@@            Coverage Diff             @@
##           master      #10      +/-   ##
==========================================
- Coverage   98.11%   95.97%   -2.15%     
==========================================
  Files           1        2       +1     
  Lines         159      273     +114     
==========================================
+ Hits          156      262     +106     
- Misses          3       11       +8

Impacted Files	Coverage Δ
src/btreemap.rs	`96.26% <96.26%> (ø)`
src/lib.rs	`95.68% <100.00%> (-2.43%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1e963db...baf5a95. Read the comment docs.

JelteF

Thanks for the work on this, but the way this PR is currently structered makes it very hard to review. Mainly because it does all kinds of refactors together with functionality changes. Can you change this PR to only do functionality changes?

Also, I just merged another PR with serde support. Please update this PR to include that as well.

JelteF · 2020-09-06T11:53:14Z

src/btreemap.rs

@@ -0,0 +1,462 @@
+/*!


This commment should only be part of the main module. It just clutters the docs if it's on every page.

JelteF · 2020-09-06T11:54:33Z

src/hashmap.rs

+ ```
+*/
+
+use std::borrow::Borrow;


Can you leave the hashmap implementation in the src/lib.rs file? Now I have no clue what you changed here, because the git diff shows it all as new.

To clarify, you want only the hashmap in src/lib.rs? (Hashmap code wasn't changed, just copied+pasted.)

yeah, I think I'll move it to a separate file after merging this like you did. But to be able to review the PR easily I'd like the changes to be minimal.

So with that the final changes should be:

Add set_default to DefaultHashMap in src/lib.rs

Add a src/btreemap.rs with your new btree code (with serde support copy pasted from defaulthashmap too).

Yep, makes sense. In retrospect, I should have made two PRs: one for DefaultBTreeMap and one for set_default.

Yeah that would have been even better, but set_default is small enough that it's fine to have them together this time.

JelteF · 2020-09-06T11:59:26Z

src/lib.rs

-//! This is a problem for the first addition when there's no value for the key yet.
-//! With this library you can specify when creating the map that the default value should be zero.
-//!
+/*!


Please leave the commenting style the same, so //! instead of /*!

facepalm Of course!

rljacobson · 2020-09-07T15:36:29Z

@JelteF Since I am revisiting this PR, I might as well "do it right." There are a few more methods from BTreeMap that I would like to implement:

append()
range()
range_mut()
split_off()

The range() (range_mut()) method requires an additional Range (resp. RangeMut) structure. It is not clear what the semantics of range() should be. It could match that of get() in the sense that any key for which the map contains no value will produce (&key, &self.default). The alternative is that it matches the behavior of keys and skips over missing keys. The latter semantics makes more sense to me. A counter-argument is that then range() and range_mut() would give a different sequence—unless you have range_mut() work differently from get_mut().

While I am at it, should I implement the missing HashMap methods? They are:

iter_mut()
remove_entry()
with_capacity()
get_key_value()

I have excluded methods related to using a custom Hasher. To be blunt, I don't know anything about how custom Hashers, so it's easier to keep kicking that can down the road until someone wants it.

And since I am thinking about this, I would also like to implement an alternative semantics indicated by a new enum with variants InsertOnRead and InsertOnWrite. But this is an entirely separate PR. I'll open an issue to discuss it.

JelteF · 2020-09-07T15:44:22Z

I would say lets do those extra methods in an upcoming PR, since there might be more discussion points.

JelteF · 2020-09-07T15:45:05Z

You could create the PR already if you want though, but I don't think it should block merging this PR.

rljacobson · 2020-09-07T19:19:23Z

Bit of a noob question, but how do I rebase my changes to the new upstream commit and keep the PR clean? Normally I would put my changes on a new branch, reset master to before my changes, git pull the upstream commits, and then rebase my new changes branch onto the updated master branch. But then to sync my github repo, wouldn't I have to git push --force? Then what happens to the PR?

JelteF · 2020-09-07T19:28:39Z

Force push to the same branch as the PR will update the PR automatically. So the flow you describe is what you want indeed.

…

On Mon, 7 Sep 2020, 21:19 Robert Jacobson, ***@***.***> wrote: Bit of a noob question, but how do I rebase my changes to the new upstream commit and keep the PR clean? Normally I would put my changes on a new branch, reset master to before my changes, git pull the upstream commits, and then rebase my new changes branch onto the updated master branch. But then to sync my github repo, wouldn't I have to git push --force? Then what happens to the PR? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#10 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAI3YJTHF5TGNV64DX6UMADSEUW4RANCNFSM4QPYSS3Q> .

rljacobson · 2020-09-07T19:33:33Z

Thanks. I'll do that and squash my commits.

…shMap` and `DefaultBTreeMap`.

JelteF · 2020-09-10T20:46:04Z

Released this as 0.5.0

JelteF requested changes Sep 6, 2020

View reviewed changes

rljacobson requested a review from JelteF September 7, 2020 19:11

Added DefaultBtreeMap and a set_default method to both `DefaultHa…

baf5a95

…shMap` and `DefaultBTreeMap`.

rljacobson force-pushed the master branch from a5b71c7 to baf5a95 Compare September 7, 2020 19:50

JelteF merged commit 17f108e into JelteF:master Sep 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added `DefaultBtreeMap` and a `set_default` method. #10

Added `DefaultBtreeMap` and a `set_default` method. #10

rljacobson commented Aug 30, 2020

codecov-commenter commented Aug 30, 2020 •

edited

Loading

JelteF left a comment

JelteF Sep 6, 2020

JelteF Sep 6, 2020

rljacobson Sep 7, 2020

JelteF Sep 7, 2020

JelteF Sep 7, 2020

rljacobson Sep 7, 2020

JelteF Sep 7, 2020

JelteF Sep 6, 2020

rljacobson Sep 7, 2020

rljacobson commented Sep 7, 2020

JelteF commented Sep 7, 2020

JelteF commented Sep 7, 2020

rljacobson commented Sep 7, 2020

JelteF commented Sep 7, 2020 via email

rljacobson commented Sep 7, 2020

JelteF commented Sep 10, 2020

Added DefaultBtreeMap and a set_default method. #10

Added DefaultBtreeMap and a set_default method. #10

Conversation

rljacobson commented Aug 30, 2020

codecov-commenter commented Aug 30, 2020 • edited Loading

Codecov Report

JelteF left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rljacobson commented Sep 7, 2020

JelteF commented Sep 7, 2020

JelteF commented Sep 7, 2020

rljacobson commented Sep 7, 2020

JelteF commented Sep 7, 2020 via email

rljacobson commented Sep 7, 2020

JelteF commented Sep 10, 2020

Added `DefaultBtreeMap` and a `set_default` method. #10

Added `DefaultBtreeMap` and a `set_default` method. #10

codecov-commenter commented Aug 30, 2020 •

edited

Loading