Skip to content

Storable events #8

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Storable events #8

wants to merge 10 commits into from

Conversation

50U10FCA7
Copy link
Contributor

Synopsis

Storing events in repository.

Solution

Implement wrapper around Event containing metadata needed to determine event.

Checklist

  • Created PR:
    • In draft mode
    • Name contains issue reference
    • Has type and k:: labels applied
    • Has assignee
  • Before review:
    • Documentation is updated (if required)
    • Tests are updated (if required)
    • Changes conform code style
    • CHANGELOG entry is added (if required)
    • FCM (final commit message) is posted or updated
    • Draft mode is removed
  • Review is completed and changes are approved
    • FCM (final commit message) is approved
  • Before merge:
    • Milestone is set
    • PR's name and description are correct and up-to-date
    • All temporary labels are removed

@50U10FCA7 50U10FCA7 added k::api Related to API (application interface) k::design Related to overall design and/or architecture labels Jan 16, 2023
@50U10FCA7 50U10FCA7 self-assigned this Jan 16, 2023
@50U10FCA7
Copy link
Contributor Author

50U10FCA7 commented Feb 16, 2023

@tyranron Found a problem with TryFrom<Raw> impl codegen. It requires us to use reflect feature, because the types we hold in es::event::codegen::Reflect can't be compared with es::event::Name and es::event::Revision.

#[automatically_derived]
impl<'__raw, __Data> TryFrom<Raw<'__raw, __Data, ()>> for Event
where
    FileEvent: TryFrom<Raw<'__raw, __Data, ()>>,
    ChatEvent: TryFrom<
        Raw<'__raw, __Data, ()>,
        Error = <FileEvent as TryFrom<Raw<'__raw, __Data, ()>>::Error
    >
{
    type Error = FromRawError<<FileEvent as TryFrom<__Data>>::Error, ()>;

    fn try_from(raw: Raw<'__raw, __Data, ()>) -> Result<Self, Self::Error> {
        for (_, var_name, var_rev) in <FileEvent as codegen::Reflect>::META {
            if var_name == raw.name && var_rev == raw.rev {  // Can't compare because `Reflect` hold values as strings.
                return <FileEvent as TryFrom<__Data>>::try_from(raw.data)
                    .map(Self::File)
                    .map_err(FromRawError::FromDataError);
            }
        }

       /* check ChatEvent */

        Err(FromRawError::UnknownEvent {
            name: raw.name.to_string(),
            revision: raw.revision,
        })
    }
}

Possible solutions:

  1. Use es::event::reflect::* instead of es::event::codegen::Reflect, but this makes reflect functionality non-optional.
  2. Add bound to Revision makes it convertible into string variant to compare with es::event::codegen::Reflect value.

@tyranron
Copy link
Member

@50U10FCA7 as the code is generated, I think we may go with string conversion.

@50U10FCA7
Copy link
Contributor Author

50U10FCA7 commented Feb 17, 2023

@tyranron Maybe we should block duplicated variants in enums, because it drives ambiguous behaviour.

#[derive(Event)]
enum MyEvent {
    #[event(init)]
    Init(SomethingHappened),
    NotInit(SomethingHappened),
}

Once enum from example above is deserialized/converted from raw it always be init.

Alternatively we can error in cases when duplicated variants have different Source-behaviour.

@50U10FCA7
Copy link
Contributor Author

50U10FCA7 commented Feb 17, 2023

@tyranron Maybe we should block duplicated variants in enums, because it drives ambiguous behaviour.

#[derive(Event)]
enum MyEvent {
    #[event(init)]
    Init(SomethingHappened),
    NotInit(SomethingHappened),
}

Once enum from example above is deserialized/converted from raw it always be init.

Alternatively we can error in cases when duplicated variants have different Source-behaviour.

Discussed: Type-level doesn't allows to use such Events later, so the block/warn is not required.

Type-check recursion when expanding from/into Raw conversion

Also discussed the problem with recursion without same variants deduplication.

For example the following code:

#[derive(Event)]
enum ParEvent {
    A(SubEvent),
    B(SubEvent),
}

will expand the following (code is simplified):

// from `event::Raw` to `ParEvent`
impl<__Data> TryFrom<Raw<__Data>> for ParEvent
where
    SubEvent: TryFrom<__Data>,
    SubEvent: TryFrom<__Data, Error = <SubEvent as TryFrom<__Data>>::Error>, // recursion
{
    type Error = <SubEvent as TryFrom<__Data>>:Error;

    fn try_from(raw: Raw<__Data>) -> Result<Self, Self::Error> {
        todo!()
    }
}

As discussed there is no way to dedup the variants properly in non type-level (in macro expansion), but I checked the following case:

type SubEvent2 = SubEvent;

#[derive(Event)]
enum ParEvent {
    A(SubEvent),
    B(SubEvent2),
}

which generates:

// from `event::Raw` to `ParEvent`
impl<__Data> TryFrom<Raw<__Data>> for ParEvent
where
    SubEvent: TryFrom<__Data>,
    SubEvent2: TryFrom<__Data, Error = <SubEvent as TryFrom<__Data>>::Error>, // no recursion!
{
    type Error = <SubEvent as TryFrom<__Data>>:Error;

    fn try_from(raw: Raw<__Data>) -> Result<Self, Self::Error> {
        todo!()
    }
}

And I think its OK to dedup exactly the same variants because only them drives recursion.

@50U10FCA7
Copy link
Contributor Author

Found an alternative to current discussed design of the event::Raw. Instead of using typed revision in it we can use a string representation of it (like we do in es::event::codegen::Reflect). It may help to abstract the event::Raw from exact Event. When casting from abstracted Raw into concrete Event we only need to check that two strings of revision are equal. This way requires change the ToString bound of es::event::Revision to FromStr.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
k::api Related to API (application interface) k::design Related to overall design and/or architecture
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants