Attempt at checking for license (#209) #2456

dlukes · 2018-02-16T23:27:11Z

I'm not quite sure how best to handle loading the license template from
a path -- I mean obviously I know how to do it, but I'm not sure where
to fit it in the codebase :) So this first attempt puts the license
template directly into the config file.

These are my misgivings about the license template config option as a
path to a file (I'd love feedback if some of these are wrong or can be
easily circumvented!):

I thought the obvious choice for the type of license_template in
create_config! should be PathBuf, but PathBuf doesn't implement
FromStr (yet? see Path & PathBuf missing FromStr trait rust#44431), so
it would have to be wrapped in a tuple struct, and I went down that road
for a little while but then it seemed like too much ceremony for too
little gain.
So a plain String then (which, mind you, also means the same
doc_hint(), i.e. <string>, not <path> or something like that). The
fact that it's a valid path will be checked once we try to read the
file.
But where in the code should the license template be read? The
obvious choice for me would be somewhere in Config::from_toml(), but
since Config is defined via the create_config! macro, that would
mean tight coupling between the macro invocation (which defines the
configuration option license_template) and its definition (which would
rely on the existence of that option to run the template loading code).
license_template could also be made a special option which is
hardwired into the macro. This gets rid of the tight coupling, but
special-casing one of the config options would make the code harder to
navigate.
Instead, the macro could maybe be rewritten to allow for config
options that load additional resources from files when the config is
being parsed, but that's beyond my skill level I'm afraid (and probably
overengineering the problem if it's only ever going to be used for this
one option).
Finally, the file can be loaded at some later point in time, e.g. in
format_lines(), right before check_license() is called. But to
face a potential IO error at so late a stage, when the source files
have already been parsed... I don't know, it doesn't feel right.

BTW I don't like that I'm actually parsing the license template as late
as inside check_license() either, but for much the same reasons, I
don't know where else to put it. If the Config were hand-rolled
instead of a macro, I'd just define a custom license_template option
and load and parse the template in the Config's init. But the way
things are, I'm a bit at a loss.

However, if someone more familiar with the project would kindly provide
a few hints as to how the path approach can be done in a way that is as
clean as possible in the context of the codebase, I'll be more than
happy to implement it! :)

topecongiro · 2018-02-17T08:10:47Z

Thank you for your PR!

I think we can have two config options for like we do in use_small_heuristics and width_heuristics. One option is exposed to users and holds a path to the license template file, and the other is not user-facing and holds the content of the template file. This way we can catch the potential IO errors while setting configs.

dlukes · 2018-02-17T21:15:47Z

Great, thanks for the tips! I'll see what I can do and report back :) And I'll also change the license template parsing code so that it can handle nested {} (= stop using regex to do that).

dlukes · 2018-02-19T16:38:01Z

Done, but the approach is slightly different from the case of use_small_heuristics / width_heuristics -- I just added a license_template: Option<Regex> field to the Config struct in the macro definition instead of making that a regular option, because that would require wrapping the Regex in a struct and writing code for (de)serializing it (can't derive it), even though that code would never actually be used. Or maybe it could be just a dummy implementation? Let me know what you think and what improvements are needed :)

nrc

This is a really great PR, thank you! I think the way of handling the options for paths/templates works pretty well. I did have a few other comments in the code, but mostly looks good.

nrc · 2018-02-23T00:31:36Z

rustfmt-config/src/config_type.rs

+                };
+                let mut license_template_str = String::new();
+                match license_template_file.read_to_string(&mut license_template_str) {
+                    Ok(_) => (),


you could use if let Err(e) = ... rather than match here.

Thanks, of course, done :)

nrc · 2018-02-23T00:33:55Z

rustfmt-config/src/config_type.rs

@@ -365,12 +373,50 @@ macro_rules! create_config {
                    self.set().width_heuristics(WidthHeuristics::null());
                }
            }
+
+            fn set_license_template(&mut self) {
+                let license_template_path = self.license_template_path();


Are you accounting for a possibly empty license_template_path somewhere?

No, well-spotted! I didn't realize calling rustfmt with no config file is not quite the same as calling with a config file which doesn't specify the license_template_path, so it didn't bite me when trying to run it.

nrc · 2018-02-23T00:35:57Z

rustfmt-config/src/config_type.rs

+                        return;
+                    }
+                }
+            }


This function would look nicer if you create a LicenseError enum with a display method for the messages and use the ? operator.

I agree it's ugly, I just thought I'd do the error handling here and avoid clutter at the call sites :) If I use ?, how far up should I let the errors bubble? set_license_template() is called by functions which return unit, I suppose I should just pattern match there and let potential errors print?

nrc · 2018-02-23T00:36:20Z

rustfmt-config/src/lib.rs

+/// "
+/// );
+/// ```
+pub fn parse_license_template(template: &str) -> Result<String, String> {


this function should be in a submodule

nrc · 2018-02-23T00:37:32Z

rustfmt-config/src/lib.rs

+    let mut parsed = String::from("^");
+    let mut buffer = String::new();
+    let mut state = State::Lit;
+    let mut linum = 1;


This kind of code usually works better if you factor the fields into a struct and make each state a method on the struct.

I've been trying to figure out how to split that slab of code into smaller pieces, that giant match expression looks daunting. My initial thought was that there must be a way to impl enum variants, so that state transitions can be neatly divided by state of origin, but it turns out there isn't :) Then I read this, which was very informative but felt slightly overkill. Maybe not after all, I'll see what I can do.

dlukes · 2018-02-26T08:44:26Z

@nrc Thank you very much for the feedback! I'll take a look at the issues and try to resolve them :)

dlukes · 2018-02-26T18:09:51Z

@nrc OK, I think I've at least attempted to address all the issues you've raised :)

I put the license-related code into a new module.
The parsing state machine is now a struct, state transitions are separate methods.
I've defined a LicenseError enum which wraps the errors which can occur inside Config::set_license_template() and put the part of the function which can result in errors inside a closure, so that ? can be used to make the code more concise, but the function doesn't have to return a Result type and error handling can remain inside it instead of having to be done at its call sites.

Let me know if that works, or if you have different ideas about some / all of it!

dlukes · 2018-02-27T14:04:34Z

I ended up getting rid of the closure in Config::set_license_template(), there might as well be a regular function in the license module which wraps these IO operations which might possibly fail (see
2d7dbc1 ).

nrc

Looks great, thanks, r+ However, it needs another rebase, sorry. I hope it is just renaming some files since we had to revert the workspaces changes.

I'm not quite sure how best to handle loading the license template from a path -- I mean obviously I know *how* to do it, but I'm not sure where to fit it in the codebase :) So this first attempt puts the license template directly into the config file. These are my misgivings about the license template config option as a path to a file (I'd love feedback if some of these are wrong or can be easily circumvented!): 1. I thought the obvious choice for the type of `license_template` in `create_config!` should be `PathBuf`, but `PathBuf` doesn't implement `FromStr` (yet? see rust-lang/rust#44431), so it would have to be wrapped in a tuple struct, and I went down that road for a little while but then it seemed like too much ceremony for too little gain. 2. So a plain `String` then (which, mind you, also means the same `doc_hint()`, i.e. `<string>`, not `<path>` or something like that). The fact that it's a valid path will be checked once we try to read the file. 3. But where in the code should the license template be read? The obvious choice for me would be somewhere in `Config::from_toml()`, but since `Config` is defined via the `create_config!` macro, that would mean tight coupling between the macro invocation (which defines the configuration option `license_template`) and its definition (which would rely on the existence of that option to run the template loading code). 4. `license_template` could also be made a special option which is hardwired into the macro. This gets rid of the tight coupling, but special-casing one of the config options would make the code harder to navigate. 5. Instead, the macro could maybe be rewritten to allow for config options that load additional resources from files when the config is being parsed, but that's beyond my skill level I'm afraid (and probably overengineering the problem if it's only ever going to be used for this one option). 6. Finally, the file can be loaded at some later point in time, e.g. in `format_lines()`, right before `check_license()` is called. But to face a potential *IO* error at so late a stage, when the source files have already been parsed... I don't know, it doesn't feel right. BTW I don't like that I'm actually parsing the license template as late as inside `check_license()` either, but for much the same reasons, I don't know where else to put it. If the `Config` were hand-rolled instead of a macro, I'd just define a custom `license_template` option and load and parse the template in the `Config`'s init. But the way things are, I'm a bit at a loss. However, if someone more familiar with the project would kindly provide a few hints as to how the path approach can be done in a way that is as clean as possible in the context of the codebase, I'll be more than happy to implement it! :)

This allows occurrences of `{` and `}` within `{}` placeholders in the template, and also for having literal `{` and `}` in the template by means of escaping (`\{`). Unbalanced, unescaped `}` at the toplevel is a syntax error which currently triggers a panic; I'll add proper error handling as I move the license template parsing code into the config parsing phase.

Don't attempt to load license_template if the path wasn't specified.

This also splits the giant state machine match expression into separate methods.

Get rid of the unncessary closure.

dlukes · 2018-03-05T12:48:06Z

Rebased :) Although maybe I should just remove the doctest for TemplateParser::parse(), since with doctest = false in Cargo.toml, it can get easily out of sync again in case of any future restructuring of the codebase...?

nrc · 2018-03-08T02:37:10Z

Thanks! I merged so you don't have to rebase again, but I think it is worth trying to get the doc test running - does something bad happen if we enable doctest in Cargo.toml? If not, we should just do that.

dlukes · 2018-03-10T08:43:54Z

Sorry, I had a busy two days :) The doctests fail with the following output:

   Doc-tests rustfmt-nightly
WARNING: src/chains.rs - chains (line 28) Code block is not currently run as a test, but will in future versions of rustdoc. Please ensure this code block is a runnable test, or use the `ignore` directive.
WARNING: src/chains.rs - chains (line 37) Code block is not currently run as a test, but will in future versions of rustdoc. Please ensure this code block is a runnable test, or use the `ignore` directive.
WARNING: src/chains.rs - chains (line 50) Code block is not currently run as a test, but will in future versions of rustdoc. Please ensure this code block is a runnable test, or use the `ignore` directive.
WARNING: src/chains.rs - chains (line 56) Code block is not currently run as a test, but will in future versions of rustdoc. Please ensure this code block is a runnable test, or use the `ignore` directive.
WARNING: src/macros.rs - macros::indent_macro_snippet (line 624) Code block is not currently run as a test, but will in future versions of rustdoc. Please ensure this code block is a runnable test, or use the `ignore` directive.
WARNING: src/macros.rs - macros::indent_macro_snippet (line 636) Code block is not currently run as a test, but will in future versions of rustdoc. Please ensure this code block is a runnable test, or use the `ignore` directive.

running 2 tests
test src/macros.rs - macros::format_lazy_static (line 867) ... FAILED
test src/config/license.rs - config::license::TemplateParser::parse (line 85) ... ok

failures:

---- src/macros.rs - macros::format_lazy_static (line 867) stdout ----
	error: cannot find macro `lazy_static!` in this scope
 --> src/macros.rs:868:1
  |
3 | lazy_static! {
  | ^^^^^^^^^^^

error: aborting due to previous error

thread 'rustc' panicked at 'couldn't compile the test', librustdoc/test.rs:298:13
note: Run with `RUST_BACKTRACE=1` for a backtrace.


failures:
    src/macros.rs - macros::format_lazy_static (line 867)

test result: FAILED. 1 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out

error: test failed, to rerun pass '--doc'

I'll see if I can figure out how to fix it.

Doctests were disabled globally because up until rust-lang#2456, they were just formatting examples which were not supposed to compile. Now that there is one runnable doctest, I disabled the other ones individually (by adding the ignore directive). I also added some empty lines around the code blocks to avoid the following warning and instead ignore the code blocks cleanly: WARNING: ... Code block is not currently run as a test, but will in future versions of rustdoc. Please ensure this code block is a runnable test, or use the `ignore` directive. See rust-lang/rust#28712 for further details.

dlukes · 2018-03-10T09:09:48Z

Turns out it was trivial (#2529) :)

dlukes mentioned this pull request Feb 16, 2018

Check for license #209

Closed

nrc reviewed Feb 23, 2018

View reviewed changes

nrc reviewed Mar 4, 2018

View reviewed changes

dlukes added 13 commits March 5, 2018 13:11

Move license template parsing into config phase

ad76741

Simplify match → if let

ead8120

Account for possibly empty license_template_path

e48d7f3

Don't attempt to load license_template if the path wasn't specified.

Move license template parsing into submodule

310c114

Refactor parsing code into struct

bbd6d9c

This also splits the giant state machine match expression into separate methods.

Fix indentation in create_config macro definition

b33451b

Shorten var names to comply with line len reqs

533d185

Add license_template_path configuration snippet

53347bc

Wrap license-related errors in enum

1db84a3

Load and compile template in proper function

085cc90

Get rid of the unncessary closure.

Make license doctest pass again

01f6527

dlukes force-pushed the feat/check-license branch from 2d7dbc1 to 01f6527 Compare March 5, 2018 12:40

nrc merged commit f0d179d into rust-lang:master Mar 8, 2018

dlukes deleted the feat/check-license branch March 9, 2018 17:48

dlukes mentioned this pull request Mar 10, 2018

Enable doctests #2529

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Attempt at checking for license (#209) #2456

Attempt at checking for license (#209) #2456

dlukes commented Feb 16, 2018

topecongiro commented Feb 17, 2018 •

edited

Loading

dlukes commented Feb 17, 2018

dlukes commented Feb 19, 2018 •

edited

Loading

nrc left a comment

nrc Feb 23, 2018

dlukes Feb 26, 2018

nrc Feb 23, 2018

dlukes Feb 26, 2018 •

edited

Loading

dlukes Feb 26, 2018

nrc Feb 23, 2018

dlukes Feb 26, 2018

nrc Feb 23, 2018

dlukes Feb 26, 2018

nrc Feb 23, 2018

dlukes Feb 26, 2018 •

edited

Loading

dlukes commented Feb 26, 2018

dlukes commented Feb 26, 2018 •

edited

Loading

dlukes commented Feb 27, 2018

nrc left a comment

dlukes commented Mar 5, 2018 •

edited

Loading

nrc commented Mar 8, 2018

dlukes commented Mar 10, 2018

dlukes commented Mar 10, 2018

Attempt at checking for license (#209) #2456

Attempt at checking for license (#209) #2456

Conversation

dlukes commented Feb 16, 2018

topecongiro commented Feb 17, 2018 • edited Loading

dlukes commented Feb 17, 2018

dlukes commented Feb 19, 2018 • edited Loading

nrc 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

dlukes Feb 26, 2018 • edited Loading

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

dlukes Feb 26, 2018 • edited Loading

Choose a reason for hiding this comment

dlukes commented Feb 26, 2018

dlukes commented Feb 26, 2018 • edited Loading

dlukes commented Feb 27, 2018

nrc left a comment

Choose a reason for hiding this comment

dlukes commented Mar 5, 2018 • edited Loading

nrc commented Mar 8, 2018

dlukes commented Mar 10, 2018

dlukes commented Mar 10, 2018

topecongiro commented Feb 17, 2018 •

edited

Loading

dlukes commented Feb 19, 2018 •

edited

Loading

dlukes Feb 26, 2018 •

edited

Loading

dlukes Feb 26, 2018 •

edited

Loading

dlukes commented Feb 26, 2018 •

edited

Loading

dlukes commented Mar 5, 2018 •

edited

Loading