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

Seq: Implement with support for -sw #125

Merged
merged 4 commits into from
Sep 1, 2020
Merged

Seq: Implement with support for -sw #125

merged 4 commits into from
Sep 1, 2020

Conversation

yehohanan7
Copy link

Initial implementation, no support for "-f" yet.

@yehohanan7 yehohanan7 mentioned this pull request Aug 31, 2020
4 tasks
Copy link
Owner

@GrayJack GrayJack left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution!! 😄

Besides these comments, could you add seq to all workspace toml files so the CI can check all for all platforms possible?

seq/Cargo.toml Outdated
build = "build.rs"
edition = "2018"
description = """
Print numbers from FIRST to LAST, in steps of INCREMENT.
Copy link
Owner

Choose a reason for hiding this comment

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

Can you use the word "display" instead?

seq/src/cli.rs Outdated
.version_message("Display version information.")
.help_short("?")
.settings(&[ColoredHelp])
.arg(Arg::with_name("FIRST INCREMENT LAST").multiple(true))
Copy link
Owner

Choose a reason for hiding this comment

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

This arg is required, isn't it?

seq/src/cli.rs Outdated
Arg::with_name("SEPERATOR")
.short("s")
.long("separator")
.help("use STRING to separate numbers (default: \\n)")
Copy link
Owner

Choose a reason for hiding this comment

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

I think the default text would be better with square bracket to be similar to the normal default text that clap uses

seq/src/main.rs Outdated Show resolved Hide resolved
seq/src/main.rs Outdated
Comment on lines 61 to 67
fn max_decimal_digits(args: &[&str]) -> usize {
args.iter().map(|v| v.len() - v.find(".").map(|pos| pos + 1).unwrap_or(v.len())).max().unwrap()
}

fn max_digits(args: &[&str]) -> usize {
args.iter().map(|v| v.find(".").unwrap_or(v.len())).max().unwrap()
}
Copy link
Owner

Choose a reason for hiding this comment

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

Are this unwrap never fail? If they never fail, can you add comments for why it don't fail? If they can fail, it should pass the information forward or be handled on the site

Copy link
Author

Choose a reason for hiding this comment

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

That's right, it should never fail. I add a comment to make it explicit.

@yehohanan7
Copy link
Author

Thanks for your contribution!!

Besides these comments, could you add seq to all workspace toml files so the CI can check all for all platforms possible?

@GrayJack all toml files updated, thanks!

@GrayJack
Copy link
Owner

GrayJack commented Sep 1, 2020

Seems good

bors r+

@bors bors bot merged commit d1ddb71 into GrayJack:dev Sep 1, 2020
@marcospb19
Copy link
Contributor

@GrayJack
Does -f need a dedicated new issue?

@GrayJack
Copy link
Owner

GrayJack commented Sep 5, 2020

Yes, probably is the best thing to do, I was just lazy these days

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.

None yet

4 participants