Skip to content
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

Implement SecurityCapabilities in rbx_types #358

Conversation

kennethloeffler
Copy link
Member

This PR implements SecurityCapabilities in rbx_types. Its functionality is very limited right now because we have almost no idea what this datatype does or how to interact with it!

I want rojo-rbx/rbx-test-files#23 merged before adding functionality for SecurityCapabilities to any other of the crates. dekko pls review it thoroughly and make sure I got it right

Copy link
Member

@Dekkonot Dekkonot left a comment

Choose a reason for hiding this comment

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

We should put this in the changelog, but this is overall a good barebones implementation for it.

We should also provide a method to get the inner value though, since we'll need it for serializing. I'll leave the exact name up to you.

@kennethloeffler
Copy link
Member Author

kennethloeffler commented Sep 15, 2023

We should also provide a method to get the inner value though, since we'll need it for serializing. I'll leave the exact name up to you.

We won't need any accessor for parsing - for reading, rbx_xml can use u64::parse and then pass the result to the SecurityCapabilities constructor, and for writing, it can use XmlEventWriter::read_characters since SecurityCapabilities implements Display. I'd prefer to keep the value field totally locked away from consumers for now since we may change the type to bitflags in the future once we understand the meaning of the bits

This also has me thinking... maybe SecurityCapabilities::new should instead be called SecurityCapabilities::from_bits?

@kennethloeffler
Copy link
Member Author

Ok, I just removed the Display impl and added a bits accessor. This is probably better than Display because if we switch to bitflags then we'd probably want to Display to change too, forward compat with bits should work out fine, and it's similar to existing bit field types like Axes and Faces

@kennethloeffler
Copy link
Member Author

kennethloeffler commented Sep 15, 2023

Should SecurityCapabilities not implement Serialize or Deserialize just yet? The format may well change in the future...

EDIT: meh, it has to if we want to be a variant. This blows!

Copy link
Member

@Dekkonot Dekkonot left a comment

Choose a reason for hiding this comment

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

I'm not thrilled with implementing Serialize/Deserialize but I suppose we have to for Variant. With that in mind, this looks fine to me.

@kennethloeffler
Copy link
Member Author

Cool, I'll put up PRs for reflection, rbx_binary, and rbx_xml later today

@kennethloeffler kennethloeffler merged commit 88f9d09 into rojo-rbx:master Sep 15, 2023
@kennethloeffler kennethloeffler deleted the rbx-types-security-capabilities branch September 15, 2023 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants