Skip to content

Conversation

GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Nov 18, 2019

I ended the scss file cleanup. To prevent having to do it once again, I added a script to check that the file continues to follow a precise format.

I also added this check into the github actions (curious to see if python 3 is already installed there or not...).

I used this script to lint the scss file:

import os
import sys


def check_scss_file(file_path):
    if not file_path.endswith(".scss"):
        return 0
    errors = 0
    with open(file_path, "r") as f:
        block_level = 0
        empty_line_before = 0
        content = f.read().split("\n")
        instructions = 0
        blocks = [0]
        i = 0
        while i < len(content):
            reset_empty_line_count = True
            reset_instructions_count = True

            line = content[i].strip()
            if line.startswith("//"):
                i += 1
                continue
            if "\t" in line:
                print("[{}:{}]: Tab character found".format(file_path, i + 1))
                errors += 1
            line = line.split("//")
            line = line[0].strip() if len(line) > 0 else ""
            if len(line) == 0:
                empty_line_before += 1
                if empty_line_before > 1:
                    print("[{}:{}]: Unexpected empty line".format(file_path, i + 1))
                    errors += 1
                reset_empty_line_count = False
            elif line.endswith("{"):
                block_level += 1
                blocks.append(0)
                if empty_line_before != 1 and instructions > 0:
                    print("[{}:{}]: Expected an empty line before".format(file_path, i + 1))
                    errors += 1
            elif line.endswith("}"):
                block_level -= 1
                blocks.pop()
                blocks[-1] += 1
                if block_level < 0:
                    print("[{}:{}]: Parsing error: block level is below 0".format(file_path, i + 1))
                    errors += 1
                if empty_line_before > 0:
                    print("[{}:{}]: Unexpected empty line before block end".format(file_path, i))
                    errors += 1
            elif line.endswith(';'):
                if block_level > 0:
                    instructions += 1
                    reset_instructions_count = False
                if block_level > 0 and empty_line_before > 0 and blocks[-1] == 0:
                    print("[{}:{}]: Unexpected empty line before this one".format(file_path, i + 1))
                    errors += 1

            if reset_empty_line_count is True:
                empty_line_before = 0
            if reset_instructions_count is True:
                instructions = 0
            i += 1
    return errors


def through_folders(path, callbacks):
    errors = 0
    for f in os.listdir(path):
        full_path = os.path.join(path, f)
        if os.path.isdir(full_path):
            errors += through_folders(full_path, callback)
        elif os.path.isfile(full_path):
            for callback in callbacks:
                errors += callback(full_path)
    return errors


def run_main():
    errors = through_folders("templates", [check_scss_file])
    if errors != 0:
        print("{} errors detected, please fix them!".format(errors))
        sys.exit(1)
    print("No error detected. Success!")


if __name__ == "__main__":
    run_main()

@GuillaumeGomez
Copy link
Member Author

pinging someone randomly: @pietroalbini

@pietroalbini pietroalbini self-assigned this Nov 20, 2019
@pietroalbini
Copy link
Member

The formatted file looks great, but I'm not sure if it's worth adding a custom linter just for scss files.

@GuillaumeGomez
Copy link
Member Author

Considering how bad the file was before I fix the formatting in 96f00d9, I'd prefer to keep things under surveillance. The check itself is really quick to run and pretty simple making it easy to maintain.

@pietroalbini
Copy link
Member

Can we use an existing formatter like prettier? I'd prefer not rolling out our own.

@GuillaumeGomez
Copy link
Member Author

I see two reasons for this:

  1. It adds an unnecessary dependency (which itself requires to have a JS environment)
  2. It's way too big for something this small

Again, not sure to really understand your issue... :-/

@pietroalbini
Copy link
Member

There are a few problems I see with any custom linter (including ./x.py tidy tbh):

  • It only covers a small part of the syntax: as far as I can see foo:1px is not linted against, without asking to write it as foo: 1px.
  • It adds maintenance overhead, as it's more code in the repository.
  • It just tells you to fix the problems yourself, while an existing linter hopefully has autoformatting that can be hooked up in the editor.

Of course fixing those problems in a custom linter requires a lot more work than what's worth, while prettier or similar just works. The JS environment is already setup in CI, and prettier is actually nice since it's just a single package without dependencies (so no npm hell).

@GuillaumeGomez
Copy link
Member Author

It only covers a small part of the syntax: as far as I can see foo:1px is not linted against, without asking to write it as foo: 1px.

I guess it can be debated but for me it's just formatting issue, not readability so completely secondary.

It adds maintenance overhead, as it's more code in the repository.

Considering I'm the one who wrote it, I don't mind keeping it up-to-date.

It just tells you to fix the problems yourself, while an existing linter hopefully has autoformatting that can be hooked up in the editor.

Well, that's the whole point of a linter. What you're talking about is a formatter and is generally a lot bigger.

Of course fixing those problems in a custom linter requires a lot more work than what's worth, while prettier or similar just works. The JS environment is already setup in CI, and prettier is actually nice since it's just a single package without dependencies (so no npm hell).

It still requires to have npm and node installed locally (which isn't my case) which are, by themselves, a hell of dependencies. ;)

@pietroalbini
Copy link
Member

I just think if we want to do this we should do it the proper way, without custom scripts.

@GuillaumeGomez
Copy link
Member Author

Ok, just kept the scss cleanup. :'(

@GuillaumeGomez GuillaumeGomez merged commit a2bfe83 into rust-lang:master Nov 20, 2019
@GuillaumeGomez GuillaumeGomez deleted the check-scss branch November 20, 2019 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants