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 internals.rs into module #340

Closed
wants to merge 1 commit into from
Closed

Split internals.rs into module #340

wants to merge 1 commit into from

Conversation

joshleeb
Copy link
Contributor

This PR splits internals.rs into the internal module with multiple files, and moves FdOptions into opts.rs.

The main motivation behind this is to move logic for constructing FdOptions out of the main function and more readable and easier to understand in the opts module. The goal will eventually to be able to write FdOptions::from(matches) and have the options constructed.

This PR splits `internals.rs` into the `internal` module with multiple
files, and moves `FdOptions` into `opts.rs`.

The main motivation behind this is to move logic for constructing
`FdOptions` out of the main function and more readable and easier to
understand in the `opts` module. The goal will eventually to be able to
write `FdOptions::from(matches)` and have the options constructed.
Copy link
Owner

@sharkdp sharkdp left a comment

Choose a reason for hiding this comment

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

This PR splits internals.rs into the internal module with multiple files, and moves FdOptions into opts.rs.

Awesome! This module has grown into a giant collection of unrelated things and a refactoring was overdue. Thank you very much.

The main motivation behind this is to move logic for constructing FdOptions out of the main function and more readable and easier to understand in the opts module. The goal will eventually to be able to write FdOptions::from(matches) and have the options constructed.

Sounds great!

This PR is hard to review in detail. I guess there are no significant changes to the parts of the code that have been moved around?

@@ -6,19 +6,22 @@
// notice may not be copied, modified, or distributed except
// according to those terms.

use ansi_term;
use std::{
Copy link
Owner

@sharkdp sharkdp Oct 10, 2018

Choose a reason for hiding this comment

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

Wow, I didn't know this was possible :-)

@sharkdp
Copy link
Owner

sharkdp commented Oct 10, 2018

Unfortunately, there are probably horrible merge conflicts now due to the merge of #339.

@joshleeb
Copy link
Contributor Author

joshleeb commented Oct 10, 2018

All good about the merge conflicts 😄

Yep there are no major code changes in this PR it's mainly just moving stuff around.

One change which might be worth mentioning though is in internals/mod.rs tests where it is changed to use the macro oss_vec! instead of vec![oss(a), ...].

@joshleeb
Copy link
Contributor Author

Closing in favor of #341

@joshleeb joshleeb closed this Oct 12, 2018
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