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

pr: Implement this command #1918

Closed
wants to merge 50 commits into from
Closed

pr: Implement this command #1918

wants to merge 50 commits into from

Conversation

MaxSem
Copy link
Contributor

@MaxSem MaxSem commented Mar 26, 2021

This PR is a resurrection of #1327 by @tilakpatidar.

Closes #1874

pr: Add -h to print custom header instead of file name

pr: Add -n to print line numbers
pr: code refactoring for references
…umbers

pr: Add -l option set number of lines

pr: Refactor opts
pr: Add -F to print line feed character for page separator
pr: Fix number of lines printed per page

pr: Fix first short page getting skipped due to page range
pr: Remove parameter header from build_options

pr: Remove unnecessary get_input_type
pr: Add test for -h option

pr: Add test for -d option
pr: Fix long name for -n

pr: Add -N first line number option

pr: Add -n[char][width] support
pr: Fix page ranges
pr: Add test for -l option

pr: Add test for -r suppress error option
pr: Print fixed padded string for each column

pr: Fix display length vs str length due to tabs
tilakpatidar and others added 16 commits March 26, 2021 14:11
pr: Remove unwanted brancing in fill_missing_lines

pr: Remove unnecessary error check

pr: Rename key to group_key in FileLine

pr: Reformat test_pr with rustfmt
pr: fix form feed

pr: Rustfmt

pr: add test for ff and -l option
pr: add -J option

pr: add -S option
pr: refactor batching of pages in pr
pr: refactor common iterator between pr and mpr

pr: remove fill lines in mpr
pr: extract recreate_arguments

pr: refactor get_line_for_printing

pr: refactor get_lines_for_printing

pr: refactor fetch_indexes generate for write_columns

pr: refactor write_columns

pr: refactor write_columns
Only the minimum needed to:
* Make everything compile without warnings
* Move files according to the new project structure
* Make tests pass
Copy link
Sponsor Contributor

@sylvestre sylvestre left a comment

Choose a reason for hiding this comment

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

it will take some time to review it but here is a few comments


type IOError = std::io::Error;

static NAME: &str = "pr";
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

Could you please follow this example to declarations the options?
https://github.com/uutils/coreutils/blob/master/src/uu/ls/src/ls.rs#L78-L80


pub fn uumain(args: impl uucore::Args) -> i32 {
let args = args.collect_str();
let mut opts = getopts::Options::new();
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

Not blocker but it would be nice to use clap instead of getopts

fn recreate_arguments(args: &Vec<String>) -> Vec<String> {
let column_page_option = Regex::new(r"^[-+]\d+.*").unwrap();
let num_regex: Regex = Regex::new(r"(.\d+)|(\d+)|^[^-]$").unwrap();
//let a_file: Regex = Regex::new(r"^[^-+].*").unwrap();
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

please remove useless comments

Copy link
Sponsor Contributor

@sylvestre sylvestre left a comment

Choose a reason for hiding this comment

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

well done!

}
}

fn print_usage(opts: &mut Options, matches: &Matches) -> i32 {
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

this can be removed when/if moved to clap

let option = value_to_parse.1;
i.parse::<usize>().map_err(|_e| {
PrError::EncounteredErrors(format!("invalid {} argument '{}'", option, i))
})
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

please add a test to cover this

}
});

let default_first_number: usize = NumberingMode::default().first_number;
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

same (test coverage)

tests/common/util.rs Show resolved Hide resolved
@@ -0,0 +1,513 @@
use crate::common::util::*;
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

please run rustfmt

@@ -0,0 +1,1343 @@
#![crate_name = "uu_pr"]
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

the file is quite long, maybe it would make sense to split it?

@sylvestre sylvestre mentioned this pull request Mar 27, 2021
@sylvestre
Copy link
Sponsor Contributor

@MaxSem ping?

@MaxSem
Copy link
Contributor Author

MaxSem commented Apr 6, 2021

Sorry, was busy doing something else. Will try to find time in the next few days.

@sylvestre
Copy link
Sponsor Contributor

@MaxSem ping? :)

@sylvestre
Copy link
Sponsor Contributor

@MaxSem ping? :)

@sylvestre
Copy link
Sponsor Contributor

@MaxSem ping?

@sylvestre
Copy link
Sponsor Contributor

moved to
#2300

@sylvestre sylvestre closed this May 30, 2021
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.

implement pr - convert text files for printing
3 participants