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

Split Stream and Struct traits into modules #2

Merged
merged 3 commits into from
Oct 20, 2017

Conversation

gushromp
Copy link
Contributor

No description provided.

@gushromp gushromp force-pushed the master branch 2 times, most recently from 9e7fbef to 9d37877 Compare October 17, 2017 12:18
@GreyCat
Copy link
Member

GreyCat commented Oct 17, 2017

@gushromp Thanks for your contribution! However, note that this Rust runtime is currently very experimental and there's no real compiler that generates output that would make use of it. If you want to pick up Rust target development, you're more than welcome to join in with ideas & code at kaitai-io/kaitai_struct#22.

I wonder if @LogicAndTrick would review this PR?

@GreyCat GreyCat requested a review from LogicAndTrick October 17, 2017 17:44
@LogicAndTrick
Copy link
Collaborator

I'm not sure about splitting each individual trait into its own module, at the very most I would add them both into one module called kaitai_struct. But if you use the runtime as a crate, then the Cargo import already provides that namespacing for you, so isn't the crate's "root module" enough? I'd at least put some pub use statements into the root namespace so users don't have to chain namespaces like use kaitai_struct::kaitai_struct::KaitaiStruct which contains a lot of redundancy.

@gushromp
Copy link
Contributor Author

gushromp commented Oct 18, 2017

@GreyCat Thanks! I've looked a bit into the issues that @LogicAndTrick has bumped into (with the references to the parent structures), and I think I have a vague idea on how to circumvent them but such a hierarchy is not pretty to implement in Rust (involves using Rc<RefCell<T>> everywhere).

@LogicAndTrick You're right, I now used pub use to reexport the stuff from the modules publicly, which would eliminate the redundant import while still keeping the code separated. But, if you feel like this adds unnecessary complexity and/or prefer keeping it as it was, I'll revert that :)

Copy link
Collaborator

@LogicAndTrick LogicAndTrick left a comment

Choose a reason for hiding this comment

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

Thanks for the changes, I think now this looks okay from my end. I'm fine with merging this in.

@GreyCat GreyCat merged commit e6c78b0 into kaitai-io:master Oct 20, 2017
@GreyCat
Copy link
Member

GreyCat commented Oct 20, 2017

Thanks, everybody!

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.

3 participants