Implement seq (#45) #63

FrancisMurillo · 2019-10-01T13:37:06Z

Implement seq.

Drafted the PR to show progress and any possible early reviews if needed.

TODO:

GrayJack · 2019-10-01T15:10:03Z

sleep/src/main.rs

@@ -11,7 +11,7 @@ fn main() {
        Some(values) => values,
        None => {
            eprintln!("sleep: Missing operand.\nTry 'sleep --help' for more information.");
-            process::exit(1);
+            process::exit(1);2


Be careful with this, remember to check compiling for the whole workspace

Okay... sorry about that. Must have commited that by accident.

GrayJack · 2019-10-01T15:54:46Z

For next commits follow the git commit message guideline please

GrayJack · 2019-10-01T17:58:29Z

seq/src/seq.yml

+  [OPTIONS] <FIRST> <INCREMENT> <LAST>
+args:
+    - FIRST:
+        help: >


When possible, use single line string

Alright, will apply that to the remaining single help values

GrayJack · 2019-10-01T18:00:48Z

seq/src/main.rs

+
+    fn next(&mut self) -> Option<Self::Item> {
+        if self.step.is_sign_positive() && self.current <= self.stop {
+            let result = self.current.clone();


f64 is Copy, so you don't need to clone

GrayJack · 2019-10-01T18:04:30Z

seq/src/main.rs

+            self.current += self.step;
+
+            Some(result)
+        } else if self.step.is_sign_negative() && self.current >= self.stop


The logic of the inside this else if is the same as the first if logic, it's that right? If so, you could just use logical OR (||) on the first if and remove this else if

Yeah, I refactored this to be one if condition since they are the same.

GrayJack · 2019-10-01T18:05:25Z

seq/src/main.rs

+
+use clap::{load_yaml, App, AppSettings};
+
+#[derive(Clone,Debug)]


Since they are all small Copy types, you could also derive Copy

FrancisMurillo · 2019-10-02T08:40:26Z

Running cargo run --bin seq 10 -1.001 1 gives me this output:

10
8.999
7.998000000000001
6.997000000000002
5.996000000000002
4.995000000000003
3.994000000000003
2.993000000000003
1.992000000000003

Which is expected when creating floating point sequences because of how floating point numbers are added or subtracted. To address this precision loss, I'm planning to use rust_decimal instead of f64 in the iteration step. Is it valid to import a dependency or no dependencies at all?

FrancisMurilloDigix · 2019-10-02T13:27:50Z

As part of the PR, I have to run the rustfmt. Running cargo fmt, it keeps giving me an error:

> cargo fmt -p seq
Could not parse TOML: expected an equals, found a newline at line 3

So checking out line 3 of rustfmt.toml, it has some trouble with the following lines:

...
# Could not parse TOML: expected an equals, found a newline at line 3
make_backup
...
# Could not parse TOML: duplicate key: `wrap_comments`
wrap_comments = true
# Could not parse TOML: invalid escape character in string: `y` at line 54
license_template_path = "Copyright {\y} Eric Shimizu Karbstein."
,,,
# invalid type: integer `2018`, expected string for key `edition`
edition = 2018

Commenting those lines, produces an error:

Warning: can't set `skip_children = false`, unstable features are only available in nightly channel.

I was able to make it run with cargo +nightly fmt. Is that valid or should I do something else to use rustfmt?

GrayJack · 2019-10-02T16:09:15Z

I think it's ok to use nightly just for formatting

Also, I'm ok with adding crates when makes sense, and your use makes sense, but try to keep on a minimum

FrancisMurilloDigix · 2019-10-08T13:04:38Z

@GrayJack I'm at a crossroad. Currently looking at implementing --format and --equal-width which is simply the printf functionality. My initial tactic was to use parse the printf format and convert it into print! format. However, Rust does not support dynamic print! formats, it must be a static string.

I came across runtime-fmt that may be a solution but requires nightly so out of the question. So another alternative is to use printf directly via libc. The tactic is to convert rust_decimal to f32 to c_double and use the format accordingly. I am conflicted whether this solution is the right direction. Another idea is to reimplement printf formatting for f32with pure Rust but might take longer.

Any suggested direction or idea for my dilemma?

GrayJack · 2019-10-08T13:24:36Z

Still, using printf from c would require unsafe, so I don't think you should use them
In the echo code, you will find a good part of formatting there, missing only \uHHHH, \UHHHHHHHH, %%, %b and %q

FrancisMurilloDigix · 2019-10-08T13:44:23Z

So I should parse the printf format and implement the display myself? Should be fun.

Using this as a reference for the format

GrayJack · 2019-10-08T14:06:59Z

Yeah, I think at one point it will be in the coreutil_core, since echo, seq and printf will use that. but for now it's ok to be a copy&paste plus additions

FrancisMurilloDigix · 2019-10-08T22:31:00Z

Sorry I did not understand, what do you mean by "copy&paste plus additions"?

GrayJack · 2019-10-08T22:44:01Z

Copy and paste the formatting code from echo, add the cases that are missing (additions)

150: Seq: Add --terminator flag r=GrayJack a=reastyn Issue #63 Co-authored-by: Tomas Rokos <[email protected]>

Francis Murillo added 2 commits September 29, 2019 22:02

Initial commit

511637a

Working clap

2ac4e17

GrayJack reviewed Oct 1, 2019

View reviewed changes

seq: Initial prototype function

d6ba949

FrancisMurillo force-pushed the cli/seq branch from 4fc24b8 to d6ba949 Compare October 2, 2019 13:12

Francis Murillo added 2 commits October 5, 2019 11:30

seq: Add custom Number type for precision safety

286cb33

seq: Add separator option

6d84a8a

reastyn mentioned this pull request Oct 3, 2021

Seq: Add --terminator flag #150

Merged

bors bot added a commit that referenced this pull request Oct 5, 2021

Merge #150

d26898f

150: Seq: Add --terminator flag r=GrayJack a=reastyn Issue #63 Co-authored-by: Tomas Rokos <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement seq (#45) #63

Implement seq (#45) #63

FrancisMurillo commented Oct 1, 2019 •

edited

Loading

GrayJack Oct 1, 2019

FrancisMurillo Oct 2, 2019

GrayJack commented Oct 1, 2019

GrayJack Oct 1, 2019

FrancisMurillo Oct 2, 2019

GrayJack Oct 1, 2019

FrancisMurillo Oct 2, 2019

GrayJack Oct 1, 2019

FrancisMurillo Oct 2, 2019

GrayJack Oct 1, 2019

FrancisMurillo Oct 2, 2019

FrancisMurillo commented Oct 2, 2019 •

edited

Loading

FrancisMurilloDigix commented Oct 2, 2019

GrayJack commented Oct 2, 2019

FrancisMurilloDigix commented Oct 8, 2019 •

edited

Loading

GrayJack commented Oct 8, 2019

FrancisMurilloDigix commented Oct 8, 2019

GrayJack commented Oct 8, 2019 •

edited

Loading

FrancisMurilloDigix commented Oct 8, 2019

GrayJack commented Oct 8, 2019


		use clap::{load_yaml, App, AppSettings};

		#[derive(Clone,Debug)]

Implement seq (#45) #63

Are you sure you want to change the base?

Implement seq (#45) #63

Conversation

FrancisMurillo commented Oct 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GrayJack commented Oct 1, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FrancisMurillo commented Oct 2, 2019 • edited Loading

FrancisMurilloDigix commented Oct 2, 2019

GrayJack commented Oct 2, 2019

FrancisMurilloDigix commented Oct 8, 2019 • edited Loading

GrayJack commented Oct 8, 2019

FrancisMurilloDigix commented Oct 8, 2019

GrayJack commented Oct 8, 2019 • edited Loading

FrancisMurilloDigix commented Oct 8, 2019

GrayJack commented Oct 8, 2019

FrancisMurillo commented Oct 1, 2019 •

edited

Loading

FrancisMurillo commented Oct 2, 2019 •

edited

Loading

FrancisMurilloDigix commented Oct 8, 2019 •

edited

Loading

GrayJack commented Oct 8, 2019 •

edited

Loading