-
Notifications
You must be signed in to change notification settings - Fork 6
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
Rust client #920
base: main
Are you sure you want to change the base?
Rust client #920
Conversation
#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] | ||
#[serde(rename_all = "camelCase")] | ||
pub enum GammaSuperGroupType { | ||
/// An alumni group (not active). | ||
Alumni, | ||
/// A committee within the IT divison. | ||
Committee, | ||
/// A society recognized by the IT division. | ||
Society, | ||
/// Functionary groups within the IT division (e.g. auditors). | ||
Functionaries, | ||
/// Gamma administrators. | ||
Admin, | ||
/// A type that of group that is not specificly supported by this client. | ||
#[serde(untagged)] | ||
Other(String), | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not agree that cthit/Gamma
should provide clients that have super group types specified. The long-term goal of Gamma is to be an account service for student divisions other than just IT, and the clients it provides should support that goal.
I'm aware that this will affect how typing works with Rust, but I still do not think that should be prioritized over the long-term goal of Gamma.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can partially agree with this as super group types are defined dynamically in Gamma 2.0 so technically all of the specified groups could be missing and only Other would exist. What I don't know however is how we can do this better, for our use it is very handy to have an enum that represents the common types that we actually use.
If we just remove the comments specifying the IT division this would be applicable to basically all other student divisions on Chalmers at least? As for the Admin type that should probably be removed completely as it is not used ether way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's talk further on a future workshop how to handle clients in general, since I think a rust client will not be the last implemented client.
647e4ba
to
86a735e
Compare
Adds a client for use in rust.
Written by @ViddeM