Normalize newlines #61

kammitama5 · 2018-07-24T04:58:05Z

No description provided.

epage

Thanks for doing this. Below I've recorded some times that will help in the future with having good commit messages.

RE Commits

Once I give the go ahead, please squash your commits. I say wait so I can see what you change as the review happens
If you use "Fixes normalize_newlines adapter predicate #45" instead of "Issue normalize_newlines adapter predicate #45", github will automatically close the issue
Treat commit messages like markdown, e.g. use an extra newline to separate paragraphs
For content, it is best to focus on the user-impact and any background for your change and less on recording what you changed, the diff tells you that. e.g. Maybe instead of "Added normalize_newlines adapter predicate", it would be "Add NormalizedPredicate adapter"

epage · 2018-07-24T14:44:27Z

src/str/normalize.rs

+use Predicate;
+
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub struct NormalizedTrimPredicate<P>


Trim is a word specific to removing whitespace. This isn't doing that right? So maybe just NormalizedPredicate?

epage · 2018-07-24T14:44:56Z

src/str/mod.rs

+#[cfg(feature = "normalized-line-endings")]
+mod normalize;
+#[cfg(feature = "normalized-line-endings")]
+pub use normalized_line_endings::normalized;


I think you mean to pub use self::normalize::NormalizedPredicate

epage · 2018-07-24T14:45:25Z

src/str/adapters.rs

@@ -111,6 +111,13 @@ where
    fn from_utf8(self) -> Utf8Predicate<Self> {
        Utf8Predicate { p: self }
    }
+    /// Returns a `NormalizedTrimPredicate` that ensures the data passed
+    /// to `Self` is trimmed and Normalized.


There should be example code in here for this new predicate

epage · 2018-07-24T14:45:53Z

src/str/adapters.rs

@@ -1,9 +1,9 @@
 use std::ffi;
 use std::fmt;
 use std::str;
-
 use Predicate;


Why was this newline removed? I prefer a style where local use are kept separate from std use

epage

Thanks! This is looking great. Just some minor stuff below

epage · 2018-07-25T15:04:44Z

src/str/adapters.rs

+    // assert_eq!(true, predicate_fn.eval("Hello World\n"));
+    // assert_eq!(true, predicate_fn.eval("Hello World\r"));
+    // assert_eq!(true, predicate_fn.eval("Hello World\r\n"));
+    // assert_eq!(false, predicate_fn.eval("Goodbye"));


Don't these need 3 /s?

Yeah, looks like these aren't be properly recognized. Looking at travis, it looks like this example isn't being run
https://travis-ci.org/assert-rs/predicates-rs/jobs/407899022

epage · 2018-07-25T15:05:06Z

src/str/adapters.rs

+    ///
+    /// ```
+    // use predicates::prelude::*;
+


Please have a /// here rather than a completely blan line

epage · 2018-07-25T15:05:19Z

src/str/adapters.rs

    #[cfg(feature = "normalized_trim")]
    fn normalized_trim(self) -> NormalizedTrimPredicate<Self> {
        NormalizedTrimPredicate { p: self }
+


Could you remove this extraneous whitespace?

epage

Almost there!

epage · 2018-07-26T14:01:29Z

src/str/normalize.rs

@@ -0,0 +1,28 @@
+use std::fmt;


forgot to point out that the crate owner wants copyright headers on the files.

epage · 2018-07-26T14:02:35Z

src/str/adapters.rs

+    ///
+    /// # Examples
+    ///
+    /// ```


Still not seeing this example in the test results
https://travis-ci.org/assert-rs/predicates-rs/jobs/408337538

Is it because its missing "rust" on the code fence?

epage · 2018-07-26T18:33:43Z

src/str/normalize.rs

@@ -13,6 +13,9 @@ use normalize_line_endings::normalized;
 use std::iter::FromIterator;

 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
+/// Predicate adapter that normalizes the newlines contained in the variable being tested.
+///
+/// This is created by `pred.normalized()`.


This is created by pred.normalized().

You dropped the d elsewhere, so this should be

This is created by pred.normalize().

epage · 2018-07-28T21:24:41Z

Great to see the tests passing!

Could you squash this down to one commit? Commits should be standalone. In this case, there isn't a clear reasoning to divide up the commits.

Also as reminder:

Once I give the go ahead, please squash your commits. I say wait so I can see what you change as the review happens
If you use "Fixes normalize_newlines adapter predicate #45" instead of "Issue normalize_newlines adapter predicate #45", github will automatically close the issue
Treat commit messages like markdown, e.g. use an extra newline to separate paragraphs
For content, it is best to focus on the user-impact and any background for your change and less on recording what you changed, the diff tells you that. e.g. Maybe instead of "Added normalize_newlines adapter predicate", it would be "Add NormalizedPredicate adapter"

Resolve adapter.rs Added normalize_newlines adapter predicate Created normalize file and added normalize-line-endings as default dependency Created normalize trim predicate Fixes assert-rs#45 Normalize newlines - Insert newline in adapter.rs for readability - Add example code in adapter.rs for usability - Add self to mod.rs - Change to NormalizedPredicate in normalize.rs to avoid ambiguity Normalize newlines syntax clean-up Make comments "///" instead of "//" for consistency Improve readability by making one commented block for function in adapter.rs Delete extraneous whitespace for readability Normalize NewLines Add copyright notice to adapters.rs and normalize.rs Amend doc tests for passing CI Normalize NewLines passes all tests Struct documentation added for NormalizedPredicate Assert tests fixed in adapters.rs (string assertions did not match) change struct description to pred.normalize() for consistency Add Cfg feature to NormalizedPredicate in adapter.rs

epage requested changes Jul 24, 2018

View reviewed changes

epage requested changes Jul 25, 2018

View reviewed changes

epage requested changes Jul 26, 2018

View reviewed changes

epage reviewed Jul 26, 2018

View reviewed changes

kammitama5 force-pushed the normalize_newlines branch 2 times, most recently from 0a12c70 to 043fb66 Compare July 28, 2018 18:30

kammitama5 force-pushed the normalize_newlines branch from 043fb66 to 57160a5 Compare July 28, 2018 21:29

kammitama5 force-pushed the normalize_newlines branch from 57160a5 to a418eef Compare July 28, 2018 21:45

epage merged commit b4659e5 into assert-rs:master Jul 28, 2018

kammitama5 deleted the normalize_newlines branch July 28, 2018 22:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Normalize newlines #61

Normalize newlines #61

kammitama5 commented Jul 24, 2018

epage left a comment

epage Jul 24, 2018

epage Jul 24, 2018

epage Jul 24, 2018

epage Jul 24, 2018

epage left a comment

epage Jul 25, 2018

epage Jul 25, 2018

epage Jul 25, 2018

epage Jul 25, 2018

epage left a comment

epage Jul 26, 2018

epage Jul 26, 2018

epage Jul 26, 2018

epage commented Jul 28, 2018

Normalize newlines #61

Normalize newlines #61

Conversation

kammitama5 commented Jul 24, 2018

epage left a 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

epage left a 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

epage left a 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

epage commented Jul 28, 2018