Skip to content
This repository has been archived by the owner on Oct 22, 2019. It is now read-only.

Refactors #2

Merged
merged 21 commits into from
Sep 30, 2019
Merged

Refactors #2

merged 21 commits into from
Sep 30, 2019

Conversation

TimonPost
Copy link
Member

@TimonPost TimonPost commented Sep 25, 2019

  • Moved documentation from book to lib.rs
  • Fixed and added examples to docstrings
  • Forbids unused_must_use warning -> error
  • Added unit tests
  • mod objectstyle -> private
  • mod styledobject -> private
  • Fixed bug with SetBg command, WinApi logic (crossterm-style Command for SetBg bug? crossterm#261)
  • Fixed bug with StyledObject, used stdout for resetting terminal color
  • Introduced ResetColor command

@TimonPost TimonPost changed the title Refactoring [WIP] Refactoring Sep 25, 2019
@zrzka
Copy link
Contributor

zrzka commented Sep 26, 2019

Please, change the ColorType back to color. No matter what name we do use internally the public one must be clear.

set_fg(ColorType::Blue)?;

I'm setting color with ColorType::Blue instead of Color::Blue. Sounds a bit weird for the crate user. Checking the code to think / propose another name.

@zrzka
Copy link
Contributor

zrzka commented Sep 26, 2019

Here's my take on it ...

  • TerminalColor is fine
  • ColorType should be Color

... and now the ...

pub(crate) trait Color : Sync + Send {
    /// Set the foreground color to the given color.
    fn set_fg(&self, fg_color: ColorType) -> Result<()>;
    /// Set the background color to the given color.
    fn set_bg(&self, fg_color: ColorType) -> Result<()>;
    /// Reset the terminal color to default.
    fn reset(&self) -> Result<()>;
}

This trait is internal, which means that it can have any name. Is the Color right name? For Color, I'd expect color manipulation methods (set R, G, B, get R, G, B, set / get alpha, ...). But these methods do not manipulate with color properties.

As we're in the -style module, so, let's follow my -cursor refactoring here.

  • src/color.rs -> src/style.rs (Color -> Style)
  • src/color -> src/style (AnsiColor -> AnsiStyle, etc.)

@TimonPost
Copy link
Member Author

Okay I agree and I prefer Color as well, I want to make sure that your okay with having twice the name name as well.

@zrzka
Copy link
Contributor

zrzka commented Sep 26, 2019

I want to make sure that your okay with having twice the name name as well.

Why twice? As I mentioned, we should rename the trait Color to trait Style (+ module) or any other name.

src/styledobject.rs Show resolved Hide resolved
src/objectstyle.rs Outdated Show resolved Hide resolved
@TimonPost TimonPost changed the title [WIP] Refactoring Refactors Sep 26, 2019
Copy link
Contributor

@zrzka zrzka left a comment

Choose a reason for hiding this comment

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

Try to learn to write a changelog entry in PR. Then you wont be forced to recreate them, go through changes, etc.

.travis.yml Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/objectstyle.rs Outdated Show resolved Hide resolved
src/objectstyle.rs Outdated Show resolved Hide resolved
src/style/winapi.rs Outdated Show resolved Hide resolved
src/style.rs Show resolved Hide resolved
src/style/winapi.rs Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
src/style/ansi.rs Outdated Show resolved Hide resolved
src/style/ansi.rs Outdated Show resolved Hide resolved
src/style/winapi.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@zrzka zrzka left a comment

Choose a reason for hiding this comment

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

I think it can be merged (except the unused_imports which should be fixed).

I'll sync & improve the documentation once this one is merged - to follow the style of the -cursor & -terminal & others. Started adding comments, but there were lot of them, so, I trashed them and will do a PR.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants