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

Feature/improve colour contrast #43

Closed
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 5 additions & 7 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,8 @@ These are the clients I tried but failed to compile or run:
-o --os <type> Override the operating system [linux, osx, sunos]
-u --update Update the local cache
-c --clear-cache Clear the local cache
-d --display-config Show config directory path
-s --seed-config Create a basic config
--display-config Show config directory path
--seed-config Create a basic config

Examples:

Expand All @@ -89,11 +89,9 @@ These are the clients I tried but failed to compile or run:

The tldr page syntax highlighting can be customized with a config file.
Creating the config file can be done manually or with the help of tldr.
```
tldr -s
```
tldr --seed-config

It should print the location where it created the config file.
The command should print the location where the command created the config file.
Example: `~/.config/tealdeer/syntax.toml`

The currently supported attributes are:
Expand All @@ -103,7 +101,7 @@ The currently supported attributes are:
- `underline`
- `bold`

The currently supported colours are:
The currently supported colors are:

- `Black`
- `Red`
Expand Down
70 changes: 39 additions & 31 deletions src/config.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
use std::env;
use std::fs;
use std::io::{Read, Write};
use std::io::{Error as IoError, Read, Write};
use std::path::PathBuf;

use ansi_term::{Colour, Style};
use ansi_term::{Color, Style};
use toml;
use xdg::BaseDirectories;

Expand All @@ -12,7 +12,7 @@ use error::TealdeerError::{self, ConfigError};
pub const SYNTAX_CONFIG_FILE_NAME: &'static str = "syntax.toml";
Voultapher marked this conversation as resolved.
Show resolved Hide resolved

#[derive(Copy, Clone, Debug, Serialize, Deserialize, PartialEq, Eq, PartialOrd, Ord)]
Voultapher marked this conversation as resolved.
Show resolved Hide resolved
pub enum RawColour {
pub enum RawColor {
Black,
Red,
Green,
Expand All @@ -23,25 +23,25 @@ pub enum RawColour {
White,
}

impl From<RawColour> for Colour {
fn from(raw_colour: RawColour) -> Colour {
match raw_colour {
RawColour::Black => Colour::Black,
RawColour::Red => Colour::Red,
RawColour::Green => Colour::Green,
RawColour::Yellow => Colour::Yellow,
RawColour::Blue => Colour::Blue,
RawColour::Purple => Colour::Purple,
RawColour::Cyan => Colour::Cyan,
RawColour::White => Colour::White,
impl From<RawColor> for Color {
fn from(raw_color: RawColor) -> Color {
match raw_color {
RawColor::Black => Color::Black,
RawColor::Red => Color::Red,
RawColor::Green => Color::Green,
RawColor::Yellow => Color::Yellow,
RawColor::Blue => Color::Blue,
RawColor::Purple => Color::Purple,
RawColor::Cyan => Color::Cyan,
RawColor::White => Color::White,
}
}
}

#[derive(Copy, Clone, Debug, Serialize, Deserialize, PartialEq, Eq, PartialOrd, Ord)]
#[derive(Copy, Clone, Debug, Serialize, Deserialize, PartialEq, Eq)]
struct RawStyle {
pub foreground: Option<RawColour>,
pub background: Option<RawColour>,
pub foreground: Option<RawColor>,
pub background: Option<RawColor>,
pub underline: bool,
pub bold: bool,
}
Expand All @@ -62,11 +62,11 @@ impl From<RawStyle> for Style {
let mut style = Style::default();

if let Some(foreground) = raw_style.foreground {
style = style.fg(Colour::from(foreground));
style = style.fg(Color::from(foreground));
}

if let Some(background) = raw_style.background {
style = style.on(Colour::from(background));
style = style.on(Color::from(background));
}

if raw_style.underline {
Expand All @@ -81,7 +81,7 @@ impl From<RawStyle> for Style {
}
}

#[derive(Debug, Default, Serialize, Deserialize, PartialEq, Eq, PartialOrd, Ord)]
#[derive(Debug, Default, Serialize, Deserialize, PartialEq, Eq)]
struct RawConfig {
pub highlight_style: RawStyle,
pub description_style: RawStyle,
Expand All @@ -94,7 +94,7 @@ impl RawConfig {
fn new() -> RawConfig {
let mut raw_config = RawConfig::default();

raw_config.highlight_style.foreground = Some(RawColour::Red);
raw_config.highlight_style.foreground = Some(RawColor::Red);
raw_config.example_variable_style.underline = true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why these defaults? Shouldn't this be the responsibility of the calling code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure, what do we gain from having the caller specify them?

Copy link
Collaborator

Choose a reason for hiding this comment

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

When creating a new RawConfig instance, I would expect it to be uninitialized. Otherwise the responsibility of the RawConfig type (encapulating style configuration) and the calling program (configuring the style of the output) are mixed -- the container all of a sudden sets styling. I'd separate those clearly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's what default does, if we move it to the caller site we'd repeat the same thing 3 times.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, let's keep it for now, but then at least add a docstring indicating that some values will be initialized. Something like this:

/// Create a new `RawConfig` instance with some values initialized to certain default values:
///
/// - Foreground highlight style is "red"
/// - Example variable style is "underlined"


raw_config
Expand All @@ -113,22 +113,28 @@ pub struct Config {
impl From<RawConfig> for Config {
fn from(raw_config: RawConfig) -> Config {
Config{
highlight_style: Style::from(raw_config.highlight_style),
description_style: Style::from(raw_config.description_style),
example_text_style: Style::from(raw_config.example_text_style),
example_code_style: Style::from(raw_config.example_code_style),
example_variable_style: Style::from(raw_config.example_variable_style),
highlight_style: raw_config.highlight_style.into(),
description_style: raw_config.description_style.into(),
example_text_style: raw_config.example_text_style.into(),
example_code_style: raw_config.example_code_style.into(),
example_variable_style: raw_config.example_variable_style.into(),
}
}
}

fn map_io_err_to_config_err(e: IoError) -> TealdeerError {
ConfigError(format!("Io Error: {}", e))
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good approach, with the helper function! 👍


impl Config {
pub fn new() -> Result<Config, TealdeerError> {
pub fn load() -> Result<Config, TealdeerError> {
let raw_config = match get_syntax_config_path() {
Ok(syntax_config_file_path) => {
let mut syntax_config_file = fs::File::open(syntax_config_file_path)?;
let mut syntax_config_file = fs::File::open(syntax_config_file_path)
.map_err(map_io_err_to_config_err)?;
let mut contents = String::new();
let _rc = syntax_config_file.read_to_string(&mut contents)?;
let _rc = syntax_config_file.read_to_string(&mut contents)
.map_err(map_io_err_to_config_err)?;

toml::from_str(&contents).map_err(|err| ConfigError(format!("Failed to parse syntax config file: {}", err)))?
}
Expand Down Expand Up @@ -192,8 +198,10 @@ pub fn make_default_syntax_config() -> Result<PathBuf, TealdeerError> {
.map_err(|err| ConfigError(format!("Failed to serialize default syntax config: {}", err)))?;

let syntax_config_file_path = config_dir.join(SYNTAX_CONFIG_FILE_NAME);
let mut syntax_config_file = fs::File::create(&syntax_config_file_path)?;
let _wc = syntax_config_file.write(serialized_syntax_config.as_bytes())?;
let mut syntax_config_file = fs::File::create(&syntax_config_file_path)
.map_err(map_io_err_to_config_err)?;
let _wc = syntax_config_file.write(serialized_syntax_config.as_bytes())
.map_err(map_io_err_to_config_err)?;

Ok(syntax_config_file_path)
}
Expand Down
8 changes: 0 additions & 8 deletions src/error.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
use std::io::Error as IoError;

use curl::Error as CurlError;

#[derive(Debug)]
Expand All @@ -14,9 +12,3 @@ impl From<CurlError> for TealdeerError {
TealdeerError::UpdateError(format!("Curl error: {}", err.to_string()))
}
}

impl From<IoError> for TealdeerError {
fn from(err: IoError) -> TealdeerError {
TealdeerError::ConfigError(format!("Io error: {}", err))
}
}
1 change: 1 addition & 0 deletions src/formatter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ pub fn print_lines<R>(tokenizer: &mut Tokenizer<R>, config: &Config) where R: Bu
// This is safe as long as the parsed title is only the command,
// and tokenizer yields values in order of appearance.
command = title;
Voultapher marked this conversation as resolved.
Show resolved Hide resolved
debug!("Detected command name: {}", &command);
},
LineType::Description(text) => println!(" {}", format_description(&text, &config)),
LineType::ExampleText(text) => println!(" {}", config.example_text_style.paint(text)),
dbrgn marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
38 changes: 19 additions & 19 deletions src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ use std::path::{Path, PathBuf};
use std::process;

use docopt::Docopt;
use ansi_term::Colour;
use ansi_term::Color;

mod types;
mod tokenizer;
Expand Down Expand Up @@ -75,8 +75,8 @@ Options:
-o --os <type> Override the operating system [linux, osx, sunos]
-u --update Update the local cache
-c --clear-cache Clear the local cache
-d --display-config Show config directory path
-s --seed-config Create a basic config
--display-config Show config directory path
--seed-config Create a basic config

Examples:

Expand Down Expand Up @@ -118,14 +118,14 @@ fn print_page(path: &Path) -> Result<(), String> {
let reader = BufReader::new(file);

// Look up config file, if none is found fall back to default config.
let config = match Config::new() {
let config = match Config::load() {
Ok(config) => config,
Err(ConfigError(msg)) => {
println!("Failed to create config: {}", msg);
eprintln!("Failed to create config: {}", msg);
process::exit(1);
}
Err(_) => {
println!("Unknown error while creating config");
eprintln!("Unknown error while creating config");
process::exit(1);
}
Voultapher marked this conversation as resolved.
Show resolved Hide resolved
};
Expand All @@ -142,14 +142,14 @@ fn check_cache(args: &Args, cache: &Cache) {
if !args.flag_update {
match cache.last_update() {
Some(ago) if ago > MAX_CACHE_AGE => {
println!("{}", Colour::Red.paint(format!(
println!("{}", Color::Red.paint(format!(
"Cache wasn't updated in {} days.\n\
You should probably run `tldr --update` soon.\n",
MAX_CACHE_AGE / 24 / 3600
)));
},
None => {
println!("Cache not found. Please run `tldr --update`.");
eprintln!("Cache not found. Please run `tldr --update`.");
process::exit(1);
},
_ => {},
Expand Down Expand Up @@ -203,7 +203,7 @@ fn main() {
cache.clear().unwrap_or_else(|e| {
match e {
CacheError(msg) | ConfigError(msg) | UpdateError(msg) =>
println!("Could not delete cache: {}", msg),
eprintln!("Could not delete cache: {}", msg),
};
process::exit(1);
});
Expand All @@ -215,7 +215,7 @@ fn main() {
cache.update().unwrap_or_else(|e| {
match e {
CacheError(msg) | ConfigError(msg) | UpdateError(msg) =>
println!("Could not update cache: {}", msg),
eprintln!("Could not update cache: {}", msg),
};
process::exit(1);
});
Expand All @@ -226,7 +226,7 @@ fn main() {
if let Some(ref file) = args.flag_render {
let path = PathBuf::from(file);
if let Err(msg) = print_page(&path) {
println!("{}", msg);
eprintln!("{}", msg);
process::exit(1);
} else {
process::exit(0);
Expand All @@ -242,7 +242,7 @@ fn main() {
let pages = cache.list_pages().unwrap_or_else(|e| {
match e {
CacheError(msg) | ConfigError(msg) | UpdateError(msg) =>
println!("Could not get list of pages: {}", msg),
eprintln!("Could not get list of pages: {}", msg),
}
process::exit(1);
});
Expand All @@ -260,15 +260,15 @@ fn main() {
// Search for command in cache
if let Some(path) = cache.find_page(&command) {
if let Err(msg) = print_page(&path) {
println!("{}", msg);
eprintln!("{}", msg);
process::exit(1);
} else {
process::exit(0);
}
} else {
println!("Page {} not found in cache", &command);
println!("Try updating with `tldr --update`, or submit a pull request to:");
println!("https://github.com/tldr-pages/tldr");
eprintln!("https://github.com/tldr-pages/tldr");
process::exit(1);
}
}
Expand All @@ -281,11 +281,11 @@ fn main() {
process::exit(0);
},
Err(ConfigError(msg)) => {
println!("Could not look up syntax_config_path: {}", msg);
eprintln!("Could not look up syntax_config_path: {}", msg);
process::exit(1);
},
Err(_) => {
println!("Unknown error");
eprintln!("Unknown error");
process::exit(1);
},
}
Expand All @@ -299,19 +299,19 @@ fn main() {
process::exit(0);
},
Err(ConfigError(msg)) => {
println!("Could not look up syntax_config_path: {}", msg);
eprintln!("Could not look up syntax_config_path: {}", msg);
process::exit(1);
},
Err(_) => {
println!("Unkown error");
eprintln!("Unkown error");
process::exit(1);
},
}
}

// Some flags can be run without a command.
if !(args.flag_update || args.flag_clear_cache) {
println!("{}", USAGE);
eprintln!("{}", USAGE);
process::exit(1);
}
}
Expand Down
6 changes: 3 additions & 3 deletions tests/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ fn test_missing_cache() {
.assert()
.with_args(&["sl"])
.fails()
.stdout().contains("Cache not found. Please run `tldr --update`.")
.stderr().contains("Cache not found. Please run `tldr --update`.")
.unwrap();
}

Expand All @@ -52,7 +52,7 @@ fn test_update_cache() {
testenv.assert()
.with_args(&["sl"])
.fails()
.stdout().contains("Cache not found. Please run `tldr --update`.")
.stderr().contains("Cache not found. Please run `tldr --update`.")
.unwrap();

testenv.assert()
Expand All @@ -72,7 +72,7 @@ fn test_setup_seed_config() {
let testenv = TestEnv::new();

testenv.assert()
.with_args(&["-s"])
.with_args(&["--seed-config"])
.succeeds()
.stdout().contains("Successfully created seed syntax config file")
.unwrap();
Expand Down