Implement Events Handling (#1) #2

ilslv · 2021-08-13T12:25:28Z

Part of #1

Synopsis

Firstly we should implement abstractions for Events

Solution

Checklist

… proc macro

ilslv · 2021-08-17T08:35:51Z

FCM

Implement Events Handling (#1, #2)

- Traits
  - Event
  - VersionedEvent
  - EventSourced
  - EventInitialised
- Structs
  - InitialEvent wrapper
  - EventVersion
- Proc macros
  - Event derive
  - VersionedEvent derive
- Toolchain
  - Makefile
  - GitHub Actions CI

tyranron

@ilslv will discuss stuff additionally in videocall.

tyranron · 2021-08-17T09:35:45Z

core/src/event/mod.rs

+    ///
+    /// _Note:_ This should effectively be a constant value, and should never
+    /// change.
+    fn event_type(&self) -> &'static str;


Let's change it to name. Termin type here was borrowed from other implementations, but tends to make more confusion. With name we have a clear separation when we're saying "event name" and "event type".

tyranron · 2021-08-17T09:37:01Z

core/src/lib.rs

+    unused_results
+)]
+
+mod event;


Let's hide it behind es (event sourcing) feature gate.

tyranron · 2021-08-17T09:37:52Z

.github/workflows/ci.yml

+  # Testing #
+  ###########
+
+  test:


Also let's add tests for MSRV (minimum supported Rust version) from the start.

tyranron · 2021-08-17T09:40:18Z

core/src/event/mod.rs

+/// Wrapper-type intended for [`Event`]s that can initialize [`Sourced`] items.
+#[derive(Debug, RefCast)]
+#[repr(transparent)]
+pub struct Initial<Ev: ?Sized>(pub Ev);


Clone, Copy and transparent Display would be useful as well.

tyranron · 2021-08-17T09:41:41Z

core/src/event/mod.rs

+#[repr(transparent)]
+pub struct Initial<Ev: ?Sized>(pub Ev);
+
+impl<Ev: Event + ?Sized, Agg: Initialized<Ev>> Sourced<Initial<Ev>>


It would be better to use S and "state" terminology here than "aggregate" as more general.

tyranron · 2021-08-17T09:43:20Z

core/src/lib.rs

+#[doc(inline)]
+pub use event::{
+    Event, Initial as InitialEvent, Initialized as EventInitialized,
+    Sourced as EventSourced, Versioned as VersionedEvent,


And let's call the module es (event sourcing).

tyranron · 2021-08-17T09:53:45Z

codegen/impl/src/event/versioned.rs

+
+use super::MAX_UNIQUE_EVENTS;
+
+/// Derives `arcana::VersionedEvent` for struct.


Use links to arcana-core crate. The crates layout allows it.

tyranron · 2021-08-17T09:54:21Z

codegen/impl/src/event/versioned.rs

+    ident: syn::Ident,
+    generics: syn::Generics,
+    event_type: syn::LitStr,
+    event_ver: syn::LitInt,


These fields should have documantation.

tyranron · 2021-08-17T09:55:21Z

codegen/impl/src/event/versioned.rs

+    r#type: Option<syn::LitStr>,
+
+    #[parse(value)]
+    version: Option<syn::LitInt>,


It would be nice to see Attrs defined before Definition, following parse -> generate intuitive logic.

tyranron · 2021-08-17T10:05:37Z

src/private.rs

+    pub const fn all_unique<const N: usize>(
+        types: [Option<(&str, u16)>; N],
+    ) -> bool {
+        const fn str_eq(l: &str, r: &str) -> bool {


It's OK for this function to be outside.

…ourcing

tyranron

@ilslv still, discussion is required.

tyranron · 2021-08-17T15:09:39Z

src/private.rs

+
+    #[doc(hidden)]
+    #[must_use]
+    const fn str_eq(l: &str, r: &str) -> bool {


No #[doc(hidden)]. Describe it. And an appropriate // TODO: for future removal.

tyranron · 2021-08-17T15:11:13Z

core/src/es/mod.rs

@@ -80,8 +79,8 @@ impl Version {
 }

 impl<Ev: Versioned> Event for Ev {


?Sized here.

tyranron · 2021-08-17T15:13:17Z

src/private.rs

    #[doc(hidden)]
    #[macro_export]
-    macro_rules! unique_event_type_and_ver_for_struct {
-        ($max_events:literal, $event_type:literal, $event_ver:literal) => {
+    macro_rules! unique_event_name_and_ver_for_struct {


This macros better move to proc-macro machinery. There is no point to generate a half of an implementation via declarative and another part via proc macros. Better to keep things in a single place asap.

tyranron

Discussed.

tyranron · 2021-08-24T09:36:31Z

codegen/core/src/event/versioned.rs

+            _ => {
+                return Err(syn::Error::new_spanned(
+                    input,
+                    "`name` and `version` arguments expected",


We have synthez::Required for that.

tyranron · 2021-08-24T09:50:39Z

codegen/core/src/event/versioned.rs

+
+            impl #impl_generics #name #ty_generics #where_clause {
+                #[automatically_derived]
+                pub const fn __arcana_events() -> [(&'static str, u16); 1] {


It's better to hide such inner machinery from docs via #[doc(hidden)].

ilslv · 2021-08-25T06:55:13Z

codegen/core/src/es/event/versioned.rs

+
+        assert_eq!(
+            format!("{}", err),
+            "either `version` or `ver` argument of `#[event]` attribute is \


@tyranron it looks like error message isn't deterministic. Sometimes I get either 'ver' or 'version' argument of '#[event]' attribute is expected to be present, but is absent, so ver and version are switched (even on 2 CI runs (first, second) for the same code different jobs failed). As I understand this happens because synthez uses HashSet<syn::Ident> under the hood. HashSet docs are saying that there is requirement

k1 == k2 -> hash(k1) == hash(k2)

And for syn::Ident that promise isn't held up as I understand. PartialEq impl compares ident itself and it's Span, while Hash impl simply hashes ident, ignoring it's Span. This can be easily fixed specifically for error message generation, but I'm not sure if this thing can cause bigger ploblems.

…ourcing

tyranron

I've accidentally started reviewing it without realizing it's still wip. Review and consider my changes.

tyranron · 2021-08-25T10:06:43Z

codegen/core/Cargo.toml

+readme = "README.md"
+
+[dependencies]
+arcana-core = { version = "0.1.0-dev", path = "../../core", features = ["es"] }


We shouldn't bloat dependencies with unused ones. If it's required for docs, let's introduce an appropriate feature, disabled by default.

tyranron · 2021-08-25T11:14:45Z

codegen/core/src/es/event/mod.rs

+
+            ::arcana::codegen::sa::const_assert!(
+                !::arcana::codegen::unique_events::has_duplicates(
+                    #name::__arcana_events()


That simply won't work with generics.

@tyranron it looks like there is no way to make it work with generics for now 😔
With your solution compiler error is

can't use generic parameters from outer function E0401
use of generic parameter from outer function

tyranron

FCM

Provide basic events sourcing (#2, #1)

- bootstrap project structure

Additionally:
- bootstrap CI pipeline
- bootstrap Makefile

tyranron · 2021-08-30T11:05:35Z

codegen/shim/src/lib.rs

+/// #[derive(Event)]
+/// enum DuplicatedEvent {
+///     Any(AnyEvent),
+///     File(FileEvent),


@ilslv it seems that exactly this situation should be legit.

I guess it should be allowed to use the same event type multiple times. This allows us easily to merge sets of events.

What should be forbidden really - is using the same event_name + event_ver pair in different Rust types.

Let's merge this PR as it is, and resolve the described issue in a separate one.

ilslv added 3 commits August 13, 2021 14:34

Basic project structure with Event and VersionedEvent impls

6697475

Add Makefile, GitHub Actions, PR and Issue templates

a6e1847

Add EventInitialized, EventSourced and InitialEvent

238058d

ilslv added feature New feature or request k::design Related to overall design and/or architecture k::toolchain Related to project toolchain labels Aug 13, 2021

ilslv added this to the 0.1.0 milestone Aug 13, 2021

ilslv self-assigned this Aug 13, 2021

ilslv changed the title ~~Draft: Implement Events Handling #1~~ Draft: Implement Events Handling (#1) Aug 13, 2021

ilslv added 10 commits August 13, 2021 15:27

Fix CI

83f9911

Finally check uniqueness of event_type() and ver() at compile-time!

f31ca0c

Add docs explaining unique checks macro shenanigans [skip ci]

42c5b4d

Add ability to skip uniqueness check for event_type and ver for Event…

7dfe985

… proc macro

Add more tests

7df18ef

Final touches

d454eaf

Use nightly toolchain on CI

c61e8bb

Fix CI

32aec5d

Remove stable and beta toolchains in testing

aa8f2b4

CHANGELOG

e815e60

ilslv requested review from tyranron and removed request for tyranron August 17, 2021 08:37

tyranron and others added 5 commits August 17, 2021 12:32

Merge branch 'master' into 1-event-sourcing

11107d1

Introduce EventName and EventVersion and support them in proc-macros

b048ca1

CHANGELOG

c6f1ff9

Merge branch 'master' into 1-event-sourcing

d7647b6

Some corrections [skip ci]

c477378

tyranron requested changes Aug 17, 2021

View reviewed changes

Merge remote-tracking branch 'origin/1-event-sourcing' into 1-event-s…

5166708

…ourcing

tyranron removed the k::toolchain Related to project toolchain label Aug 17, 2021

Cover VersionedEvent derive macro with docs [skip ci]

32f233f

tyranron requested changes Aug 17, 2021

View reviewed changes

ilslv added 6 commits August 18, 2021 11:40

Move events uniqueness check to arcana_codegen crate [skip ci]

18a0c1a

Use only required number of events in uniqueness check [skip ci]

de5edea

Add arcana-codegen-shim [skip ci]

fb0260a

Add MSRV check

d74ae8e

Fix CI

eff3a0a

Remove safety_guard dependency

373d0c1

ilslv requested a review from tyranron August 23, 2021 06:59

Some corrections [skip ci]

4ad217b

tyranron requested changes Aug 24, 2021

View reviewed changes

Corrections

088db1a

ilslv commented Aug 25, 2021

View reviewed changes

ilslv mentioned this pull request Aug 25, 2021

Fix non-deterministic error messages arcane-rs/synthez#2

Merged

16 tasks

ilslv and others added 4 commits August 25, 2021 10:53

Correction

8cea01b

Disable -Z minimal-versions for now on CI MSRV check

0a40645

Some corrections [skip ci]

8a84f85

Merge remote-tracking branch 'origin/1-event-sourcing' into 1-event-s…

23306f4

…ourcing

tyranron requested changes Aug 25, 2021

View reviewed changes

ilslv added 3 commits August 26, 2021 10:12

Try to support generics in internal uniqueness glue code

2d2c92d

Hide docs dependencies behind the feature

843cae5

Corrections

7685c03

ilslv requested a review from tyranron August 26, 2021 07:23

Corrections

f522fcf

tyranron changed the title ~~Draft: Implement Events Handling (#1)~~ Implement Events Handling (#1) Aug 30, 2021

tyranron marked this pull request as ready for review August 30, 2021 10:59

tyranron approved these changes Aug 30, 2021

View reviewed changes

tyranron merged commit 5a5e7ed into master Aug 30, 2021

tyranron deleted the 1-event-sourcing branch August 30, 2021 11:06

ilslv mentioned this pull request Aug 31, 2021

Check that every combination of event::Name and event::Version correspond to single Rust type (#1) #3

Merged

16 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Events Handling (#1) #2

Implement Events Handling (#1) #2

ilslv commented Aug 13, 2021 •

edited by tyranron

Loading

ilslv commented Aug 17, 2021 •

edited

Loading

tyranron left a comment

tyranron Aug 17, 2021

tyranron Aug 17, 2021

tyranron Aug 17, 2021

tyranron Aug 17, 2021

tyranron Aug 17, 2021

tyranron Aug 17, 2021

tyranron Aug 17, 2021

tyranron Aug 17, 2021

tyranron Aug 17, 2021

tyranron Aug 17, 2021

tyranron left a comment

tyranron Aug 17, 2021

tyranron Aug 17, 2021

tyranron Aug 17, 2021

tyranron left a comment

tyranron Aug 24, 2021

tyranron Aug 24, 2021

ilslv Aug 25, 2021 •

edited

Loading

tyranron left a comment •

edited

Loading

tyranron Aug 25, 2021

tyranron Aug 25, 2021

ilslv Aug 25, 2021

tyranron left a comment

tyranron Aug 30, 2021


		use super::MAX_UNIQUE_EVENTS;

		/// Derives `arcana::VersionedEvent` for struct.

		@@ -80,8 +79,8 @@ impl Version {
		}

		impl<Ev: Versioned> Event for Ev {

Implement Events Handling (#1) #2

Implement Events Handling (#1) #2

Conversation

ilslv commented Aug 13, 2021 • edited by tyranron Loading

Synopsis

Solution

Checklist

ilslv commented Aug 17, 2021 • edited Loading

FCM

tyranron 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

Choose a reason for hiding this comment

tyranron 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

tyranron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilslv Aug 25, 2021 • edited Loading

Choose a reason for hiding this comment

tyranron left a comment • edited Loading

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

tyranron left a comment

Choose a reason for hiding this comment

FCM

Choose a reason for hiding this comment

ilslv commented Aug 13, 2021 •

edited by tyranron

Loading

ilslv commented Aug 17, 2021 •

edited

Loading

ilslv Aug 25, 2021 •

edited

Loading

tyranron left a comment •

edited

Loading