OpType -> FlatOp + some reorg #1909

maackle · 2023-02-10T00:55:16Z

Summary

OpType doesn't seem like a descriptive name for what it is. I needed to reorganize some code so decided to try another name. We can pick another name. Consider this an open call for new names!

This also splits up the large holochain_integrity_types::op module, moving the "FlatOp" code downstream into hdi, which will help for the further logic reworking I'm doing as a followup.

TODO:

CHANGELOG(s) updated with appropriate info
Just before pressing the merge button, ensure new entries to CHANGELOG(s) are still under the UNRELEASED heading

jost-s

Good stuff!

Did you know that the README of the hdi is auto-generated on the release CI, based on the hdi's lib's crate doc comments? Or else you can generate it by running scripts/generate_readmes.sh.

jost-s · 2023-02-10T02:33:40Z

crates/hdi/src/op.rs

    /// This will clone the required internal data.
-    fn to_type<ET, LT>(&self) -> Result<OpType<ET, LT>, WasmError>
+    fn flattened<ET, LT>(&self) -> Result<FlatOp<ET, LT>, WasmError>


In line with the Rust method flatten for Iterators, I suggest

Suggested change

fn flattened<ET, LT>(&self) -> Result<FlatOp<ET, LT>, WasmError>

fn flatten<ET, LT>(&self) -> Result<FlatOp<ET, LT>, WasmError>

I explicitly wanted to not collide with the name used in Iterator, since this is in a completely different sense

Mh, is it? Iterators are flattened from x-dimensional arrays to x-1-dimensional arrays so to say.
Here the multi-depth enums are flattened to one level of depth. Is that completely different?

Maybe not completely different, but certainly not equivalent. We're modifying the enum in the sense of flattening it like nested arrays, but it's not a strict interleaving of one layer into another. We're sometimes bubbling up helpful things from a few layers down, sometimes expanding one variant to two (in the case of Entry and PrivateEntry), but not expanding the sibling variants. It's a very customized flattening operation, unlike the well-defined automatic flatten.

Ok, I understand.

But then I gotta ask, do you think that flattened instead of flatten conveys this difference?

I'd rather have this run the risk of being mistaken for a mere flattening of the enum, than using a past tense verb which looks more like a naming negligence than indicating a substantial difference.

jost-s · 2023-02-10T02:33:52Z

crates/hdi/src/op.rs

    where
        ET: EntryTypesHelper + UnitEnum,
        <ET as UnitEnum>::Unit: Into<ZomeEntryTypesKey>,
        LT: LinkTypesHelper,
        WasmError: From<<ET as EntryTypesHelper>::Error>,
        WasmError: From<<LT as LinkTypesHelper>::Error>;
+
+    /// Alias for `flattened`, for backward compatibility
+    #[deprecated = "`to_type` has been renamed to `flattened`, please use that name instead"]


Suggested change

#[deprecated = "`to_type` has been renamed to `flattened`, please use that name instead"]

#[deprecated = "`to_type` has been renamed to `flatten`, please use that name instead"]

jost-s · 2023-02-10T02:34:11Z

crates/hdi/src/op.rs

@@ -63,7 +76,7 @@ enum ActivityEntry<Unit> {
 }

 impl OpHelper for Op {
-    fn to_type<ET, LT>(&self) -> Result<OpType<ET, LT>, WasmError>
+    fn flattened<ET, LT>(&self) -> Result<FlatOp<ET, LT>, WasmError>


Suggested change

fn flattened<ET, LT>(&self) -> Result<FlatOp<ET, LT>, WasmError>

fn flatten<ET, LT>(&self) -> Result<FlatOp<ET, LT>, WasmError>

guillemcordoba · 2023-02-10T11:05:04Z

Is this going to be merged into holochain 0.1? It is a breaking change to the hdi, which means a bump in the hdi to 0.3 right? I don't think it's a breaking change in anything else.

maackle · 2023-02-10T20:43:09Z

@guillemcordoba unless I missed something, the type aliases should make this a non-breaking change.

guillemcordoba · 2023-02-10T22:00:50Z

Ohhh nice! We should be good then :)

Connoropolous · 2023-02-13T22:34:45Z

I'm just going to abstain, thanks for the request for input 👍

mattyg · 2023-02-14T03:43:22Z

These are my choice picks from theasaurus.com:

ClearOp
LucidOp
OpHumanized
CoherentOp
LegibleOp
NeatOp
IntelligibleOp
SuccinctOp

* Failing test * Fix test * WIP big refactor * Fix all compiler errors * Add FlatOps for storing cap entries * Fix private entry type logic * Test that sort of works * Test passes * Changelog * Removed needless test cases * Fix some warnings * More specific error * Relax test case * Remove dbg!s and backtraces * Apply suggestions from code review Co-authored-by: Jost Schulte <jost.schulte@protonmail.com> --------- Co-authored-by: Jost Schulte <jost.schulte@protonmail.com>

abe-njama · 2023-03-23T10:32:33Z

@maackle should we backport this fix into 0.1.x?

* OpType -> FlatOp * Reorg * Reorg * Move flat_op to hdi * Tightening up OpType/FlatOp logic (#1910) * Failing test * Fix test * WIP big refactor * Fix all compiler errors * Add FlatOps for storing cap entries * Fix private entry type logic * Test that sort of works * Test passes * Changelog * Removed needless test cases * Fix some warnings * More specific error * Relax test case * Remove dbg!s and backtraces * Apply suggestions from code review Co-authored-by: Jost Schulte <jost.schulte@protonmail.com> --------- Co-authored-by: Jost Schulte <jost.schulte@protonmail.com> --------- Co-authored-by: Jost Schulte <jost.schulte@protonmail.com>

* Fix clone cell idempotency for enable/disable (#2093) * Fix old tokio dep * Failing test and fixes of clone cell enable/disable idempotency * Changelog * Update comment * Differentate between CellMissing and CellDisabled, and other fixes (#2092) * Fix old tokio dep * Possible fix to intermittent disabled apps * Clear all ephemeral functions at once * Make cell_by_id differentiate between reasons for cell missing * Revert app status check and add detailed comment * Fix test * Clippy * Update comment for ListCellIds * Changelog * Take `name` out of DnaDefHash (#2099) * Take `name` out of DnaDefHash Fixes #2038 * Changelog * Update tests * Update crates/holochain_zome_types/CHANGELOG.md Co-authored-by: Jost Schulte <jost.schulte@protonmail.com> --------- Co-authored-by: Jost Schulte <jost.schulte@protonmail.com> * refactor(zome-call): optimize cap grant verification (#2097) (#2116) * refactor(zome-call): cap grant verification * refactor: simplify cap grant validation flow * update changelog * simplify assigned cap access arm * add PR reference to changelog * add comment to test * Apply suggestions from code review * refactor: use high level db query fn * replace hardcoded cap access variant by method --------- Co-authored-by: Michael Dougherty <maackle.d@gmail.com> * fix(conductor-api): reject duplicate clone cells (#1997) * add failing test for duplicate clone cell * reject duplicate dna hash * add duplicate dna hash error * specify error and test again after disabling clone * update changelog * add test for two agents cloning cells in same conductor * check for duplicate cell id in conductor state * OpType -> FlatOp + some reorg (#1909) * OpType -> FlatOp * Reorg * Reorg * Move flat_op to hdi * Tightening up OpType/FlatOp logic (#1910) * Failing test * Fix test * WIP big refactor * Fix all compiler errors * Add FlatOps for storing cap entries * Fix private entry type logic * Test that sort of works * Test passes * Changelog * Removed needless test cases * Fix some warnings * More specific error * Relax test case * Remove dbg!s and backtraces * Apply suggestions from code review Co-authored-by: Jost Schulte <jost.schulte@protonmail.com> --------- Co-authored-by: Jost Schulte <jost.schulte@protonmail.com> --------- Co-authored-by: Jost Schulte <jost.schulte@protonmail.com> * fix typo * fix conductor tests * AppManifest `version` becomes `installed_hash` (#2157) * Make `version` take only a single optional hash * Rename AppManifest's `version` -> `installed_hash` * Changelog * Look up local DNAs before resolving via Location * Include AppManifest in AppInfo (#2156) * Return manifest with AppInfo and rewrite legacy app install * Clippy * update source 'nixpkgs' on branch 'develop' (#2158) [create-pull-request] automated change Co-authored-by: steveeJ <steveeJ@users.noreply.github.com> * update source 'rust-overlay' on branch 'develop' (#2159) [create-pull-request] automated change Co-authored-by: steveeJ <steveeJ@users.noreply.github.com> * fix(release-automation): consider dependency state of all matched crates (#2160) fix(release-automation): consider dependency state of all matched crates (#2149) * fix(release-automation): consider dependency state of all matched crates the change filter could lead to dependency changes not including the changed dependency in the release, if the selected crate itself didn't have any changed. the blocked filter was removed because it adds unnecessary obfuscation. * chore(release-automation): remove duplicate test * test(relase-automation): cover the fixed case * chore: add FIXME comment for a bug in the dependencies_in_workspace_filter function * test(release-automation): extend dependency bump test to cover the transitive case * refactor(release-automation/crate_selection): clarify transitive change detection * update source 'nixpkgs' on branch 'develop' (#2161) [create-pull-request] automated change Co-authored-by: steveeJ <steveeJ@users.noreply.github.com> * Test that manifest includes most recent modifiers * Investigating BadChecksum * Update from_raw_32 to only allow 32 * Use 32 byte hash for a test where it matters * Fix fixturator * Fix one more test --------- Co-authored-by: Holochain Release Automation <100725712+holochain-release-automation2@users.noreply.github.com> Co-authored-by: steveeJ <steveeJ@users.noreply.github.com> Co-authored-by: Stefan Junker <1181362+steveeJ@users.noreply.github.com> --------- Co-authored-by: Holochain Release Automation <100725712+holochain-release-automation2@users.noreply.github.com> Co-authored-by: steveeJ <steveeJ@users.noreply.github.com> Co-authored-by: Stefan Junker <1181362+steveeJ@users.noreply.github.com> * Add backward compat test and fix existing manifest serde test * Rename to `_version` * Fix visibility * Fix bad backport * Tests that network seed always affects dna hash (#2102) * Take `name` out of DnaDefHash Fixes #2038 * Changelog * Write passing test for reported bug #2039 * Add tests for paths -- still passes * Extremely comprehensive test of network seed and location: still passes! * Even more comprehensive test: still passes! * Small fries, wip * Added more cases; still passes * Better debug * Add regression test and fix the problem * Changelog * Update crates/holochain/src/conductor/tests/install_app_bundle.rs Co-authored-by: Jost Schulte <jost.schulte@protonmail.com> --------- Co-authored-by: Jost Schulte <jost.schulte@protonmail.com> * Fix test * Improve HoloHash::from_raw_32 and use it (#2162) * Update from_raw_32 to only allow 32 * Use 32 byte hash for a test where it matters * Fix fixturator * Fix one more test * Fix test --------- Co-authored-by: Jost Schulte <jost.schulte@protonmail.com> Co-authored-by: Holochain Release Automation <100725712+holochain-release-automation2@users.noreply.github.com> Co-authored-by: steveeJ <steveeJ@users.noreply.github.com> Co-authored-by: Stefan Junker <1181362+steveeJ@users.noreply.github.com>

maackle added 4 commits February 9, 2023 14:10

OpType -> FlatOp

2d7869a

Reorg

bee46c8

Reorg

b2c86d9

Move flat_op to hdi

cabd445

maackle requested review from zippy, jost-s, Connoropolous and guillemcordoba February 10, 2023 01:19

jost-s approved these changes Feb 10, 2023

View reviewed changes

maackle and others added 2 commits February 21, 2023 11:01

Merge branch 'develop' into flat-op

57cb32d

maackle enabled auto-merge (squash) February 21, 2023 21:37

maackle merged commit 1bdf77e into develop Feb 21, 2023

maackle deleted the flat-op branch February 21, 2023 21:42

abe-njama added the ShouldBackport label Mar 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OpType -> FlatOp + some reorg #1909

OpType -> FlatOp + some reorg #1909

maackle commented Feb 10, 2023 •

edited

jost-s left a comment

jost-s Feb 10, 2023

maackle Feb 10, 2023

jost-s Feb 10, 2023

maackle Feb 13, 2023

jost-s Feb 13, 2023

jost-s Feb 10, 2023

jost-s Feb 10, 2023

guillemcordoba commented Feb 10, 2023

maackle commented Feb 10, 2023

guillemcordoba commented Feb 10, 2023

Connoropolous commented Feb 13, 2023

mattyg commented Feb 14, 2023 •

edited

abe-njama commented Mar 23, 2023

	fn flattened<ET, LT>(&self) -> Result<FlatOp<ET, LT>, WasmError>
	fn flatten<ET, LT>(&self) -> Result<FlatOp<ET, LT>, WasmError>

	#[deprecated = "`to_type` has been renamed to `flattened`, please use that name instead"]
	#[deprecated = "`to_type` has been renamed to `flatten`, please use that name instead"]

OpType -> FlatOp + some reorg #1909

OpType -> FlatOp + some reorg #1909

Conversation

maackle commented Feb 10, 2023 • edited

Summary

TODO:

jost-s left a comment

Choose a reason for hiding this comment

jost-s Feb 10, 2023

Choose a reason for hiding this comment

maackle Feb 10, 2023

Choose a reason for hiding this comment

jost-s Feb 10, 2023

Choose a reason for hiding this comment

maackle Feb 13, 2023

Choose a reason for hiding this comment

jost-s Feb 13, 2023

Choose a reason for hiding this comment

jost-s Feb 10, 2023

Choose a reason for hiding this comment

jost-s Feb 10, 2023

Choose a reason for hiding this comment

guillemcordoba commented Feb 10, 2023

maackle commented Feb 10, 2023

guillemcordoba commented Feb 10, 2023

Connoropolous commented Feb 13, 2023

mattyg commented Feb 14, 2023 • edited

abe-njama commented Mar 23, 2023

maackle commented Feb 10, 2023 •

edited

mattyg commented Feb 14, 2023 •

edited