Script license header check and update in rust #2067
Conversation
masih
force-pushed
the
masih/check-license
branch
from
edddbe3
to
41b7798
Compare
The previous license header update, implemented in bash, would duplicate the headers if run. The rules defined in that script also seem to have been inconsistently applied. Having said that it is not clear what the desired license header rule for this repo is. The changes here implement the license check and update in rust. It runs far faster, is more maintainable, does not duplicate the license header if already present and would add it if it is missing. The rules programmed in the rust implementation are specifically picked to reduce the churn in code, and hopefully by the grace of reviewers, learn what the intended rules for this repo should be.
masih
force-pushed
the
masih/check-license
branch
from
41b7798
to
6823eeb
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2067 +/- ##
==========================================
- Coverage 75.72% 75.46% -0.26%
==========================================
Files 153 154 +1
Lines 15677 15731 +54
==========================================
Hits 11871 11871
- Misses 3806 3860 +54
|
I think the idea is:
The rules are mostly consistent for new files, although they can end up duplicating lines on occasion.
I'm concerned that in this case, especially because this is rust, the cost of compiling likely exceeds the cost of running them (unless already cached). Even with the cache, It's only slightly faster to use rust and wouldn't even compare with a little bit of bash optimization. |
|
If we just want to make it faster: #2068 |
| @@ -0,0 +1,12 @@ | |||
| [package] | |||
| name = "check-license" | |||
| version.workspace = true | |||
There was a problem hiding this comment.
I'd use a separate version here and specify "publish = false".
| file.write_all(license.as_bytes())?; | ||
| file.write_all(b"\n")?; |
There was a problem hiding this comment.
I'd usually write this as:
writeln!(&file, "{license}")?;String formatting uses compiler magic to do most of the work at compile time, so it'll be very fast.
There was a problem hiding this comment.
Although, if you want performance a bufwriter might be in order.
| println!("Adding license header to: {}", path.display()); | ||
|
|
||
| let file_content = fs::read_to_string(path)?; | ||
| let mut file = OpenOptions::new().write(true).truncate(true).open(path)?; |
There was a problem hiding this comment.
note: this doesn't actually need to be mutable because Write is implemented for &File (a trick to allow multiple concurrent writers without locking).
| .collect(); | ||
|
|
||
| let mut missing_licenses: Vec<&str> = Vec::new(); | ||
| for (regex, is_required, text_to_add) in LICENSE_CHECKS.iter() { |
There was a problem hiding this comment.
| for (regex, is_required, text_to_add) in LICENSE_CHECKS.iter() { | |
| for (regex, is_required, text_to_add) in &LICENSE_CHECKS { |
(does the same thing)
| } | ||
| } | ||
|
|
||
| if !found_match && *is_required { |
There was a problem hiding this comment.
why not skip early if not required?
| let mut found_match = false; | ||
| for line in &lines { | ||
| if regex.is_match(line) { | ||
| found_match = true; | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
let found_match = lines.iter().find(|&l|regex.is_match(l)).is_some()| if !missing_licenses.is_empty() { | ||
| println!("Adding license header to: {}", path.display()); | ||
|
|
||
| let file_content = fs::read_to_string(path)?; |
There was a problem hiding this comment.
nit: if you're just going to write it back to a file, you might as well read it as bytes.
| file.write_all(b"\n")?; | ||
| } | ||
|
|
||
| file.write_all(file_content.as_bytes())?; |
There was a problem hiding this comment.
General note: there's no need to mimic cat here. You can just:
- Open the file.
- Write at the beginning.
- Close it.
| continue; | ||
| } | ||
|
|
||
| if path.is_file() && path.extension().and_then(|s| s.to_str()) == Some("rs") { |
There was a problem hiding this comment.
| if path.is_file() && path.extension().and_then(|s| s.to_str()) == Some("rs") { | |
| if path.is_file() && path.extension().map(|e| e == "rs").unwrap_or(false) { |
(slightly shorter and &OsStr == &str is valid, IIRC)
| /// checks all `.rs` files within the specified directory, skipping any directories that are listed | ||
| /// in the `IGNORED_DIRECTORIES` set. If a required license is missing, it is automatically added | ||
| /// to the beginning of the file. | ||
| fn main() { |
There was a problem hiding this comment.
Note: main can return errors.
| /// IGNORED_DIRECTORIES is a static set containing directories to ignore during the license check. | ||
| static ref IGNORED_DIRECTORIES: HashSet<PathBuf> = { | ||
| let mut set = HashSet::new(); | ||
| set.insert(PathBuf::from("./target/")); |
There was a problem hiding this comment.
| set.insert(PathBuf::from("./target/")); | |
| set.insert("./target/".into()); |
Rust is all about the magic (when it works).
There was a problem hiding this comment.
Also, it looks like this is being used as a vec.
|
Thanks Steven, I have got more than I wanted out of this PR. I'm specifically grateful for random rust comments 🍻 |
The previous license header update, implemented in bash, would duplicate the headers if run. The rules defined in that script also seem to have been inconsistently applied. Having said that it is not clear what the desired license header rule for this repo is.
The changes here implement the license check and update in rust. It runs far faster, is more maintainable, does not duplicate the license header if already present and would add it if it is missing.
The rules programmed in the rust implementation are specifically picked to reduce the churn in code, and hopefully by the grace of reviewers, learn what the intended rules for this repo should be.