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

Remove unecessary modules in mod.rs files #313

Open
Superhepper opened this issue Jan 19, 2022 · 4 comments
Open

Remove unecessary modules in mod.rs files #313

Superhepper opened this issue Jan 19, 2022 · 4 comments
Labels
breaking change Fixing this will be a breaking change enhancement New feature or request

Comments

@Superhepper
Copy link
Collaborator

Superhepper commented Jan 19, 2022

Extra modules such as:

mod lists;
pub use self::digest_list::DigestList;
pub mod digest_list {
    pub use super::lists::digest::*;
}

Does not serve any real purpose and should be removed.

@Superhepper Superhepper added the enhancement New feature or request label Jan 19, 2022
@Superhepper Superhepper changed the title Remove unecessary paths in mod.rs files Remove unecessary modules in mod.rs files Jan 19, 2022
@ionut-arm
Copy link
Member

Do you reckon we should just keep the type exports and not make the modules public, to keep the import tree flat?

@Superhepper
Copy link
Collaborator Author

Yeah, just make everything available under structures like it already is. There is no need to provide alternative module 'paths' to the same thing for us. We do not have like backward compatibility to take into consideration here. I think I was the one introduced this a long time ago because the standard library did this. But for us it makes no sense.

@ionut-arm
Copy link
Member

ionut-arm commented Jan 20, 2022

Hmm, just noticed something odd. For some reason there's a separate module for non-volatile-related structures. Any reason for that? They seem very similar to the stuff we have in structures.

Would it make sense for me to move them around to structures?

Another question that doesn't really belong in this thread, but I don't want to open a new issue just for it - we have NvPublic, which is very similar to Public, but which currently has conversions to TPM2B_NV_PUBLIC instead of TPMS_NV_PUBLIC (like we had before for Public). Since the change to bring this in line with how Public works would only be additive (we need to implement a NvPublicBuffer, add some conversions both to this new one and to NvPublic, implement marshalling/unmarshalling), is it ok if I only move the types to structures, but otherwise skip these implementations for a future minor release?
NOTE: I'm aware the spec has different sections for the NV structures, but that's part of what makes the spec really confusing. The Public/Private structures are also in a separate section that we nonetheless included in structures. It's all a big mess, I'm just hoping we can keep it more contained.

cc @Superhepper @wiktor-k

@Superhepper
Copy link
Collaborator Author

I think the reason for the nv module is because that is how it is laid out in the specification. How the code should be laid out in our source tree is something I have been pondering about for a long time. The reason I have not been able to come up with a good answer is because we want the modules to intuitive to people who uses the crate and reads the specification. So to some degree we have laid things out as the specification has. But the specification in it self is not consistent!! Sometimes it places things under a specific type topic and sometimes it places the types under subsection of structures even though they are perhaps interface types (rant is over).

So would it make sense to move nv to structures? If we look at the specification then the answer is no, but I wouldn't say anything if it ended up there any way.

@Superhepper Superhepper added the breaking change Fixing this will be a breaking change label Mar 11, 2022
@ionut-arm ionut-arm added this to the `tss-esapi` v8.0.0 milestone Jun 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Fixing this will be a breaking change enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants