[CXX-973] Rename bsoncxx::builder::stream::finalize to extract Created: 14/Jul/16 Updated: 08/Jan/24 Resolved: 03/Mar/17 |
|
| Status: | Closed |
| Project: | C++ Driver |
| Component/s: | API |
| Affects Version/s: | None |
| Fix Version/s: | None |
| Type: | New Feature | Priority: | Major - P3 |
| Reporter: | David Golden | Assignee: | David Golden |
| Resolution: | Won't Fix | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Epic Link: | PM-329 |
| Description |
|
Because of the name, the finalize helper might easily be misunderstood as "finishing" a document being constructed, whereas it actually destructively steals the constructed document and returns it, leaving the builder with empty state. Consider the following example:
The result is:
If we rename finalize to something more self-documenting (and alarming) like steal_document, the API would be harder to mistakenly misuse. For example:
That is obviously a mistake. It also makes clear what assignment is doing:
|
| Comments |
| Comment by David Golden [ 03/Mar/17 ] |
|
As we're de-emphasizing the stream builder, I'm closign this as won't fix. |
| Comment by Andrew Morrow (Inactive) [ 01/Feb/17 ] |
|
I recommend just using in the new name as an alias of the old, or vice-versa, whatever the name may be, so you don't break existing code. |
| Comment by David Golden [ 12/Sep/16 ] |
|
I'm open to an alternate name that is more self-documenting. Possibly extract or extract_document might be clearer, and we can link the docs to core::extract_document. That has some nice congruence between builders. I also considered "release", but while the builder is releasing control, the actual return value is the new owner. |
| Comment by Mira Carey [ 12/Sep/16 ] |
|
You're acting on the document stream builder and you're stealing the underlying byte buffer that the builder owns. We're tunneling down to core::extract_document(), which calls impl::steal_document, which calls bson_destroy_with_steal, for what it's worth |
| Comment by Andrew Morrow (Inactive) [ 12/Sep/16 ] |
|
I'm not entirely convinced that I want to make this change. I think the issue is that the concept is just tricky, and I don't think the proposed new name makes it any clearer. From what are you 'stealing'? You aren't even acting on the 'document' object when you put this type - you are acting on the 'type stack'. |
| Comment by Andrew Morrow (Inactive) [ 15/Jul/16 ] |
|
If we rename it, we should using in the old name as an alias with the BSONCXX_DEPRECATED tag (I assume the deprecated atttribute works on aliases/typedefs), so that we don't break user code, and then update our examples and docs. mira.carey@mongodb.com - thoughts on whether we should find a better name, and, if so, what? I don't love 'steal', but I don't have a better suggestion at the moment. |