Skip to content

Divided analyzing logic into submodules. #105

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

Merged
merged 1 commit into from
Aug 6, 2018

Conversation

data-pup
Copy link
Member

@data-pup data-pup commented Aug 4, 2018

This breaks the different parts of analyze.rs into separate files.

The different analyses are grouped together in analyze::commands, to differentiate them from the files that are responsible for formatting output into text tables or JSON, which are found in analyze::formats. The different analyses, i.e. top, diff, and so forth are re-exported from analyze.rs using a pub use statement.

Aside from organizational work, this commit as few changes as possible to the code itself. The only changes made are:

  • For the sake of differentiating between imports coming from other crates within the project, the extern crate statements in analyze.rs do not alias the other twiggy crates. These are aliased when they are imported within the commands files, so that no other code changes.
  • Imports at the top of files within analyze::commands are divided into 3 groups, following advice from this issue. Imports from standard libraries come first, followed by imports from external crates, and imports from other crates in the project coming third.
  • The with_header and add_row methods for the formats::Table struct have had the pub keyword added, so they can be used in the commands submodule. This module is not publicly exported from the analyze crate however, so I don't believe this has an effect on their visibility elsewhere.

@fitzgen
Copy link
Member

fitzgen commented Aug 6, 2018

The with_header and add_row methods for the formats::Table struct have had the pub keyword added, so they can be used in the commands submodule. This module is not publicly exported from the analyze crate however, so I don't believe this has an effect on their visibility elsewhere.

Just to be double sure, we can mark these as pub(crate) fn blah()

Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

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

Thanks @data-pup!

My one and only nitpick is that "commands" are what the user does, but this crate is a bit removed from the user and is performing different kinds of analyses.

I am being totally unreasonable, but I would really prefer if the commands submodule were renamed to analyses :-p

Thanks again! This is a great code organization clean up!

@fitzgen
Copy link
Member

fitzgen commented Aug 6, 2018

r+ with that change, feel free to merge when its done!

@data-pup
Copy link
Member Author

data-pup commented Aug 6, 2018

am being totally unreasonable, but I would really prefer if the commands submodule were renamed to analyses :-p

Not unreasonable at all! Funnily enough, I had named that submodule analyses at the outset, but wasn't originally sure about it. Glad to know my instinct was right! I'll take care of this, and the pub(crate) detail mentioned above, and merge this after CI passes 😄

Thanks for the review!

@data-pup data-pup merged commit c7d0f79 into rustwasm:master Aug 6, 2018
@data-pup data-pup deleted the analyze-modules branch August 20, 2018 16:07
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