Refer to top-level persistence namespaces as primary_namespace #2612
Conversation
Personally I've always found the overload of a prelude enum to be confusing, and never bothered to handle it properly in bindings as a result. To avoid needing to do so now, we simply move the newly-introduced `io::Result` usages over to `Result<_, io::Error>`.
This fixes a bindings build error as `namespace` is a C++ keyword which cannot be used as an argument, and while this could be fixed in the bindings rather than here, separating the term `namespace` between the concept (which refers to the primary and sub namespaces) and the primary namespace makes the documentation more readable.
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2612 +/- ##
==========================================
+ Coverage 88.98% 89.00% +0.02%
==========================================
Files 113 113
Lines 86291 86872 +581
Branches 86291 86872 +581
==========================================
+ Hits 76787 77323 +536
- Misses 7267 7296 +29
- Partials 2237 2253 +16
☔ View full report in Codecov by Sentry. |
|
Mainly in lightning-persister crate, fs_store & utils.rs docs etc. |
Yes, only the trait method variable names get copied, internal code or trait implementations don't matter. I'll fix them all though. |
|
Done, I think. |
TheBlueMatt
force-pushed
the
2023-09-namespace-split
branch
from
738c6f9 to
623a9ce
Compare
TheBlueMatt
force-pushed
the
2023-09-namespace-split
branch
from
623a9ce to
f704f2e
Compare
| /// The sub-namespace under which the [`ChannelManager`] will be persisted. | ||
| pub const CHANNEL_MANAGER_PERSISTENCE_SUB_NAMESPACE: &str = ""; | ||
| /// The primary namespace under which the [`ChannelManager`] will be persisted. | ||
| pub const CHANNEL_MANAGER_PERSISTENCE_PRIMARY_NAMESPACE: &str = ""; |
There was a problem hiding this comment.
femtonit: These names are getting kinda long. Maybe we don't need PERSISTENCE in these constants at all, given that they're defined in persist.rs.
There was a problem hiding this comment.
Yeah, granted PERSISTENCE is a bit long, but still nice to have something in the const name that expresses what they are about, in particular as they may be used in contexts more or less unrelated to persistence . No strong opinion either way.
There was a problem hiding this comment.
Eh, they're exposed, and in bindings we don't get modules (cause most languages dont have them) so its kinda nice to have them be fully qualified.
|
LGTM! |
With the top-level namespace now called "primary", "secondary" makes more sense than "sub".
Update various variables, error strings, and the pending changelog entry to refer to new namespace terminology.
TheBlueMatt
force-pushed
the
2023-09-namespace-split
branch
from
f704f2e to
47e1148
Compare
|
Squashed with one more 100-char fix: $ git diff-tree -U1 f704f2e7 47e11482
diff --git a/lightning/src/util/persist.rs b/lightning/src/util/persist.rs
index 08039ffc3..2a022c37c 100644
--- a/lightning/src/util/persist.rs
+++ b/lightning/src/util/persist.rs
@@ -105,4 +105,4 @@ pub trait KVStore {
///
- /// Will create the given `primary_namespace` and `secondary_namespace` if not already present in the
- /// store.
+ /// Will create the given `primary_namespace` and `secondary_namespace` if not already present
+ /// in the store.
fn write(&self, primary_namespace: &str, secondary_namespace: &str, key: &str, buf: &[u8]) -> Result<(), io::Error>; |
This fixes a bindings build error as
namespaceis a C++ keywordwhich cannot be used as an argument, and while this could be fixed
in the bindings rather than here, separating the term
namespacebetween the concept (which refers to the primary and sub
namespaces) and the primary namespace makes the documentation more
readable.