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

feat(linter): add line-length rule #232

Merged
merged 15 commits into from
Feb 20, 2025

Conversation

felipeasimos
Copy link
Contributor

The idea of this PR is to add a rule that checks if the line length is below a given threshold. Currently this PR only shows a warning if there is a line with more than 120 columns (hardcoded), without specifying which line.

@github-actions github-actions bot added the A-linter Area - linter and lint rules label Feb 14, 2025
@felipeasimos
Copy link
Contributor Author

btw, just new-rule line-length didn't create src/linter/rules/line_length.zig automatically, I had to copy the examples manually

@felipeasimos felipeasimos mentioned this pull request Feb 14, 2025
Copy link
Owner

@DonIsaac DonIsaac 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 the PR! Some requested changes are blocked by things I need to do, which I'll add now. Please make sure you add docs, then run just ready and commit the changes.

}

pub fn runOnLine(_: *const LineLength, line: Line, ctx: *LinterContext) void {
if (line.text.len < 120) return;
Copy link
Owner

Choose a reason for hiding this comment

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

Add this to the rule as a field const LineLength = struct { max_length: u32 = 120 }, that way when I add rule-specific configs it will just work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really sure where you would like me to put this/ how to get this value inside the runOnce function

Copy link
Owner

@DonIsaac DonIsaac Feb 18, 2025

Choose a reason for hiding this comment

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

Check UnsafeUndefined for an example. it's config is set inside the struct itself.

pub const LineLength = struct {
  max_length: u32 = 120,
};

Then in runOnce:

pub fn runOnce(self: *const LineLength, ctx: *LinterContext) void {
  const len = getLineLength();
  if (len < self.max_len) { ... }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe I added this properly, but I can't really test it. How would a zlint.json with an option for unsafe-undefined look like?

Copy link
Owner

Choose a reason for hiding this comment

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

Eventually (not now):

{
    "rules": {
        "max-lines": ["warn", { "max-length": 100 }]
    }
}

@DonIsaac
Copy link
Owner

DonIsaac commented Feb 16, 2025

btw, just new-rule line-length didn't create src/linter/rules/line_length.zig automatically, I had to copy the examples manually

I can't repro, when I run just new-rule foo I get this:
image

EDIT: make sure you have Bun installed.

@github-actions github-actions bot added the A-reporter Area - Error reporting and formatting label Feb 17, 2025
@felipeasimos
Copy link
Contributor Author

EDIT: make sure you have Bun installed.

Yeah, sorry, that was probably due to Bun. It failed silently so I didn't realize.

Copy link

codecov bot commented Feb 17, 2025

Codecov Report

Attention: Patch coverage is 93.54839% with 2 lines in your changes missing coverage. Please review.

Project coverage is 86.76%. Comparing base (395dd28) to head (b6ad31f).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/linter/rules/line_length.zig 93.10% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #232      +/-   ##
==========================================
+ Coverage   86.72%   86.76%   +0.04%     
==========================================
  Files          69       70       +1     
  Lines        4640     4671      +31     
==========================================
+ Hits         4024     4053      +29     
- Misses        616      618       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -255,6 +255,7 @@ const std = @import("std");
const mem = std.mem;
const util = @import("util");
const _rule = @import("rule.zig");
const _span = @import("../span.zig");
Copy link
Owner

Choose a reason for hiding this comment

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

Remove dead code (zlint --fix-dangerously)

@DonIsaac DonIsaac changed the title add line length rule feat(linter) add line-length rule Feb 19, 2025
@DonIsaac DonIsaac changed the title feat(linter) add line-length rule feat(linter): add line-length rule Feb 19, 2025
undo line struct commit (not used)

remove Line from span.zig (not needed)

removed more unused imports from line_length.zig
Copy link
Owner

@DonIsaac DonIsaac left a comment

Choose a reason for hiding this comment

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

Once spans are fixed we can merge this. They should cover the entire line, not just characters >= the max length.

@felipeasimos felipeasimos marked this pull request as ready for review February 20, 2025 11:53
@felipeasimos
Copy link
Contributor Author

I see that the tests in Windows are breaking. The rule takes into account that the newline in windows would be "\r\n", but that's probably not the case here, making it so std.mem.splitSequence doesn't match "\r\n" to split the source file. So in windows, when reading a file with "\n" as newline, this would mean that the whole source file would be seem as one giant line.

I've made it so the type of newline being used is detected using the first line in the file.

@DonIsaac
Copy link
Owner

That's a much better solution, since that will work on git bash. Thank you!

@DonIsaac DonIsaac merged commit 83d016d into DonIsaac:main Feb 20, 2025
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-linter Area - linter and lint rules A-reporter Area - Error reporting and formatting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants