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

Basic Linter #41

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Basic Linter #41

wants to merge 2 commits into from

Conversation

alanliu2009
Copy link
Collaborator

Creating a preliminary linter, currently only checks for 80-character lines.

checks 80-character limit

Includes TODO to create ownership check
@alanliu2009 alanliu2009 requested a review from atcupps January 7, 2024 01:02
Copy link
Owner

@atcupps atcupps left a comment

Choose a reason for hiding this comment

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

LGTM so far but two questions

}

// go through all arguments (names of files to read)
for (idx,fname) in args().enumerate() {
Copy link
Owner

Choose a reason for hiding this comment

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

Question: Does this require you to manually input every file you want checked? If so, that's fine for now, but the goal is to be able to input a folder like /site and have it check every .ts and .svelte file without having to manually write them out.

version = "0.1.0"
edition = "2021"

[dependencies]
Copy link
Owner

Choose a reason for hiding this comment

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

Question: Why are these included in the dependencies? They are necessary for datagen but I don't think they're necessary for this linter. If these were automatically included then that's alright

Copy link
Collaborator Author

@alanliu2009 alanliu2009 Jan 7, 2024

Choose a reason for hiding this comment

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

Automatically included, likely because of the previous one.

@atcupps
Copy link
Owner

atcupps commented Jan 7, 2024

Also, personally I think it makes sense to wait to merge this PR until the ownership check is done.

Ok(text) => text
};

if text.len() > 80 {
Copy link
Owner

Choose a reason for hiding this comment

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

Question: Does this count the number of characters in a line or the number of bytes? I remember this being an issue for one of the CMSC 330 projects. I think we should be counting the number of characters, counting multi-byte Unicode characters as one char since that's how they're displayed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was the way we counted characters, the length is 1:1 with each character if I recall correctly.

@atcupps atcupps added C - site Component: `site` A - quality Area: Code or algorithm quality labels Jan 7, 2024
@atcupps
Copy link
Owner

atcupps commented Jan 7, 2024

By the way, you didn't have to do this in Rust if you'd rather do it in something like Python or another language. Entirely up to you

@atcupps atcupps added this to the 2. Release low priority milestone Jan 24, 2024
@atcupps atcupps removed this from the 2. Release low priority milestone Mar 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A - quality Area: Code or algorithm quality C - site Component: `site`
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants