Skip to content

Add client secret and session ID identifier types. - #640

Merged
jplatte merged 6 commits into
ruma:nextfrom
Frinksy:sid-secret-identifiers
Jun 23, 2021
Merged

Add client secret and session ID identifier types.#640
jplatte merged 6 commits into
ruma:nextfrom
Frinksy:sid-secret-identifiers

Conversation

@Frinksy

@Frinksy Frinksy commented Jun 20, 2021

Copy link
Copy Markdown
Contributor

Resolves: #517

I'm not sure if I've gone the right way about adding the types. I used the opaque_identifier! macro since the new types were similar to the KeyName type. Would it be better to define them manually and/or add constructors to them and validate them simultaneously?

@Frinksy
Frinksy requested review from iinuwa and jplatte as code owners June 20, 2021 13:20
@jplatte

jplatte commented Jun 20, 2021

Copy link
Copy Markdown
Member

If there is validation to be done, it should work like ServerName, not KeyName. Maybe we should have a similar macro for that, or adjust opaque_identifier! to support this case.

@Frinksy
Frinksy force-pushed the sid-secret-identifiers branch from 0933865 to cd98469 Compare June 20, 2021 23:43
@Frinksy

Frinksy commented Jun 20, 2021

Copy link
Copy Markdown
Contributor Author

If the changes I made are more in line with how it should be, then I think it would be a good idea to use a macro for it. I haven't ever written macros before, but I would be happy to try. Would it be just adding a macro like opaque_identifier with an extra argument being the validator function?

@jplatte jplatte left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some comments, but nothing fundamental. Please go ahead with creating the macro 🙂

Would it be just adding a macro like opaque_identifier with an extra argument being the validator function?

Exactly 👍🏼

Comment thread crates/ruma-client-api/src/r0/account/add_3pid.rs Outdated
Comment thread crates/ruma-identifiers/src/client_secret.rs Outdated
Comment thread crates/ruma-identifiers/src/client_secret.rs Outdated
Comment thread crates/ruma-identifiers/src/client_secret.rs Outdated
Comment thread crates/ruma-identifiers/src/client_secret.rs Outdated
Comment thread crates/ruma-identifiers/src/macros.rs Outdated
Comment thread crates/ruma-identifiers/src/macros.rs Outdated
Comment thread crates/ruma-identifiers/src/macros.rs Outdated
Comment thread crates/ruma-identifiers/src/macros.rs Outdated
Comment thread crates/ruma-identifiers/src/session_id.rs Outdated

@jplatte jplatte left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a rebase (should be relatively easy).

Comment thread crates/ruma-identifiers/src/macros.rs Outdated
@Frinksy
Frinksy force-pushed the sid-secret-identifiers branch from 57fa4f3 to 70c5811 Compare June 23, 2021 13:44
@Frinksy
Frinksy force-pushed the sid-secret-identifiers branch from 70c5811 to fd097bb Compare June 23, 2021 13:54
@jplatte
jplatte changed the base branch from main to next June 23, 2021 14:02
@jplatte

jplatte commented Jun 23, 2021

Copy link
Copy Markdown
Member

I've updated the base branch since this is a breaking change (for ruma-identity-service-api).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Create identifier types for client and session identifiers

2 participants