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

Refactor modules into files #98

Merged
merged 5 commits into from
Oct 21, 2019
Merged

Refactor modules into files #98

merged 5 commits into from
Oct 21, 2019

Conversation

andymac-2
Copy link
Contributor

@andymac-2 andymac-2 commented Oct 20, 2019

Fixes #97

The program is split into several modules, the largest (the info module) is already logically quite simple, and it would not make much sense to split it any further in my opinion.

I may have broken something, but as far as I can tell through my testing, this refactor went quite smoothly.

It might be good to further refactor the Info::new method to have less arguments.

I also ran rustfmt on the resulting code since I'm changing pretty much every line anyway.

@o2sh
Copy link
Owner

o2sh commented Oct 20, 2019

Why did you remove the truncate test ?

The Info module is indeed quite large.

Anyway, this is already well organized 🥇

@andymac-2 I'll let you merge your PR after the conflicts are resolved.

Thanks again!

@andymac-2
Copy link
Contributor Author

In response to the removal of the truncate test. Consider the following test:

assert_eq!(
    Tokens("  {1} {5}  {9} a").render(&colors_shim, 4, 10),
    "\u{1b}[37m\u{1b}[0m\u{1b}[37m\u{1b}[0m\u{1b}[37m \u{1b}[0m\u{1b}[37m a\u{1b}[0m   "
);

Which used to look something like...

assert_eq!(Tokens("  {1} {5}  {9} a").truncate(4, 10), "{1}{5} {9} a");

...in a previous version of the code that had a different truncate function. The old version is easily verifiable by a human being. The new version is basically unverifiable. I ended up copying the test values from the console (which is lazy) and trusting that it was correct. That means that the test wasn't doing anything productive: it would fail every time I would change something, even if the change happened to be correct. Thus the tests were removed. Perhaps they can be replaced with tests for the tokenizer which should be verifiable by a human.

@andymac-2
Copy link
Contributor Author

andymac-2 commented Oct 20, 2019

Don't merge this just yet, I've still got to update the tests.

EDIT: Done

@andymac-2
Copy link
Contributor Author

Okay, that should be it. The tests have been updated.

@o2sh o2sh merged commit ae693b3 into o2sh:master Oct 21, 2019
@o2sh
Copy link
Owner

o2sh commented Oct 21, 2019

Oops, I went too fast.
@andymac-2 You seem to have gotten rid of my central_pad (and double line break at the end):

        let center_pad = "   ";
        loop {
            match (logo_lines.next(), info_lines.next()) {
                (Some(logo_line), Some(info_line)) => 
                    writeln!(f, "{}{}{:^}", logo_line, center_pad, info_line)?,
                (Some(logo_line), None) => 
                    writeln!(f, "{}", logo_line)?,
                (None, Some(info_line)) =>
                    writeln!(f, "{:<width$}{}{:^}", "", center_pad, info_line, width = logo_lines.width())?,
                (None, None) => { writeln!(f, "\n")?; break },
            }
        };

@andymac-2
Copy link
Contributor Author

Oh whoops, that was definitely a victim of a large merge. Those commits occurred while I was moving things to different modules. As far as I can tell there are no other changes that were rolled back.

@o2sh
Copy link
Owner

o2sh commented Oct 21, 2019

No problem, merges are the worst.

Plz push the correction when possible.

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.

[Code organization] Split main.rs into modules
2 participants