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

(S)CSS files get no indention #66

Closed
GitRon opened this issue Sep 30, 2022 · 23 comments
Closed

(S)CSS files get no indention #66

GitRon opened this issue Sep 30, 2022 · 23 comments

Comments

@GitRon
Copy link

GitRon commented Sep 30, 2022

Hi there, just checked out this packge after the DjangoCon recommendation. It looks very nice but I guess I found an issue.

The djcss is removing any indention of my SCSS files which makes it more or less unreadable.

grafik

Any ideas how I can fix this? Is there a setting to adjust the indention?

Thx!

@JaapJoris
Copy link
Member

Hi there, and thanks for contributing a bug report!

DjCSS should indent both CSS and SCSS files correctly. Could you tell me the exact steps you took that lead to the SCSS file being incorrectly indented? Could you also post the output of the following command?

djcss filename.scss

@GitRon
Copy link
Author

GitRon commented Oct 4, 2022

Thanks for your quick reply! Hm, that's odd. When I use this command, the output on the CLI looks correct 🤔 What might be breaking the process when using pre-commit?

  - repo: https://github.com/rtts/djhtml
    rev: v1.5.2
    hooks:
      - id: djhtml
        # Indent only HTML files in template directories
        files: .*/templates/.*\.html$
      - id: djcss
        # Run this hook only on SCSS files (CSS and SCSS is the default)
        types: [ scss ]
      - id: djjs
        # Exclude JavaScript files in vendor directories
        exclude: .*/node_modules/.*

@JaapJoris
Copy link
Member

JaapJoris commented Oct 14, 2022

I have created a new directory with the following two files:

# pre-commit-config.yaml
- repo: https://github.com/rtts/djhtml
  rev: 'main'  # replace with the latest tag on GitHub
  hooks:
    - id: djhtml
      # Indent only HTML files in template directories
      files: .*/templates/.*\.html$
    - id: djcss
      # Run this hook only on SCSS files (CSS and SCSS is the default)
      types: [scss]
    - id: djjs
      # Exclude JavaScript files in vendor directories
      exclude: .*/node_modules/.*

and

/* file.scss */
.pic {
    float: left;
    clear: both;
    margin-right: 20px;
    margin-bottom: 15px;
}

.text {
    text-align: justify;
}
}
}

.icon-remove, .icon-pencil {
    cursor: pointer;
}

And then ran the following commands:

git init
pre-commit install
pre-commit run

The results is exactly what should be expected:

DjHTML...............................................(no files to check)Skipped
DjCSS....................................................................Passed
DjJS.................................................(no files to check)Skipped

By the way, to answer your original question, yes, there is a setting to adjust the indentation called tabwidth, which defaults to 4.

@GitRon
Copy link
Author

GitRon commented Oct 21, 2022

@JaapJoris But the file looks really ugly, the brackets are intented in a wrong way... So there is an issue, right?

I'd expect something like this:

.pic {
  float: left;
  clear: both;
  margin-right: 20px;
  margin-bottom: 15px;
}

.text {
  text-align: justify;
}

.icon-remove, .icon-pencil {
  cursor: pointer;
}

The rules are not intented in your example. And the outer brackets - if we have any - are all on the same level.

@JaapJoris
Copy link
Member

I am confused. The example you posted is equal to the example I posted. The only difference is the tabwidth (2 vs. 4 spaces). So what exactly is DjCSS doing wrong?

@GitRon
Copy link
Author

GitRon commented Oct 21, 2022

@JaapJoris Sorry, my bad! I got something wrong. I took your setup and tested it again and it's not working. And AFAIK I have no additional configuration for this:

Config

  - repo: https://github.com/rtts/djhtml
    rev: 'main'  # replace with the latest tag on GitHub
    hooks:
      - id: djhtml
        # Indent only HTML files in template directories
        files: .*/templates/.*\.html$
      - id: djcss
        # Run this hook only on SCSS files (CSS and SCSS is the default)
        types: [scss]
      - id: djjs
        # Exclude JavaScript files in vendor directories
        exclude: .*/node_modules/.*

Output

.post {
margin-top: 20px;
clear: both;

.pic {
float: left;
clear: both;
margin-right: 20px;
margin-bottom: 15px;
}

.text {
text-align: justify;
}
}

You see that there are no identations whatsoever. Really odd how this can work for you... Where would I set the tabwidth manually? Putting it directly in the yaml configuration didn't work.

@JaapJoris
Copy link
Member

Alright, I tried it again with your second example. I copied the following into file.scss:

.post {
margin-top: 20px;
clear: both;

.pic {
float: left;
clear: both;
margin-right: 20px;
margin-bottom: 15px;
}

.text {
text-align: justify;
}
}

Then, using the above pre-commit-config.yaml, I ran the following commands:

git add .
pre-commit run

This gave the following output:

DjHTML...............................................(no files to check)Skipped
DjCSS....................................................................Failed
- hook id: djcss
- files were modified by this hook

reindented file.scss
1 template has been reindented.

DjJS.................................................(no files to check)Skipped

And when I re-open file.scss in my editor, it now looks like this:

.post {
    margin-top: 20px;
    clear: both;

    .pic {
        float: left;
        clear: both;
        margin-right: 20px;
        margin-bottom: 15px;
    }

    .text {
        text-align: justify;
    }
}

Could you repeat these steps and post the output?

@GitRon
Copy link
Author

GitRon commented Oct 26, 2022

@JaapJoris ok, I did the same (in an empty repo)....

PS C:\workspace\python-playground> pre-commit run --all-files --hook-stage push
DjHTML...............................................(no files to check)Skipped
DjCSS....................................................................Passed
DjJS.................................................(no files to check)Skipped

Super weird, right? Why is it passing?

@JaapJoris
Copy link
Member

Could you also post the contents of file.scss after the pre-commit hook has run?

@GitRon
Copy link
Author

GitRon commented Oct 27, 2022

Sure, no changes. If I format the file "properly" in advance, the pre-commit will fail and change the file to this.

.post {
margin-top: 20px;
clear: both;

.pic {
float: left;
clear: both;
margin-right: 20px;
margin-bottom: 15px;
}

.text {
text-align: justify;
}
}.post {
margin-top: 20px;
clear: both;

.pic {
float: left;
clear: both;
margin-right: 20px;
margin-bottom: 15px;
}

.text {
text-align: justify;
}
}

@JaapJoris
Copy link
Member

You're right, that is really weird. The only way I can reproduce your problem is to manually override the tabwidth to 0 in .pre-commit-config.yaml:

- repo: https://github.com/rtts/djhtml
  rev: 'main'
  hooks:
    - id: djcss
      entry: djcss -i --tabwidth 0

Maybe you can try different tabwidths there and see if they have effect?

@JaapJoris
Copy link
Member

And just to confirm, the following command indents the file correctly, right?

djcss -i --tabwidth 4 file.scss

@GitRon
Copy link
Author

GitRon commented Oct 27, 2022

Hi @JaapJoris - I tried it, no effect. To run the tool on the CLI, I need the executable? My shell is not finding djcss when I call it without pre-commit.

Here's a link to my playground btw: https://github.com/GitRon/python-playground

@JaapJoris
Copy link
Member

To install DjHTML simply run pip install djhtml. This should make the djcss command available.

Thanks for the link to your playground! I ran the following commands:

$ git clone https://github.com/GitRon/python-playground.git
Cloning into 'python-playground'...
remote: Enumerating objects: 5, done.
remote: Counting objects: 100% (5/5), done.
remote: Compressing objects: 100% (4/4), done.
remote: Total 5 (delta 0), reused 5 (delta 0), pack-reused 0
Receiving objects: 100% (5/5), done.
$ cd python-playground
$ pre-commit run --all
DjHTML...............................................(no files to check)Skipped
DjCSS....................................................................Failed
- hook id: djcss
- files were modified by this hook

reindented test.scss
1 template has been reindented.

DjJS.................................................(no files to check)Skipped

And the end result was a correctly indented test.scss file. I have no idea what could cause your file to be indented with no spaces, really weird. Could you maybe run the above commands on another computer/OS to determine whether something is off about your setup?

@GitRon
Copy link
Author

GitRon commented Oct 31, 2022

Ok, tried it on a different physical machine - same behaviour. Other python version, same OS: Win10. Maybe this could be related to the issue?

@JaapJoris
Copy link
Member

In order to fix this issue, the first step is to reproduce it. Can you confirm that the following 3 commands should reproduce the issue?

git clone https://github.com/GitRon/python-playground.git
cd python-playground
pre-commit run --all

@GitRon
Copy link
Author

GitRon commented Oct 31, 2022

Yes, they do. 👍

@JaapJoris
Copy link
Member

Alright, in that case we need outside help. To anyone reading this: please try the above commands and report whether the resulting test.scss file is indented correctly 🙏

@JaapJoris JaapJoris added the help wanted Extra attention is needed label Nov 2, 2022
@golekmichal
Copy link

Hi there!
I could not reproduce this either. I tried the above commands, and the template test.scss got indented correctly.
I've checked these commands and my output is:

.post {
    margin-top: 20px;
    clear: both;

    .pic {
        float: left;
        clear: both;
        margin-right: 20px;
        margin-bottom: 15px;
    }

    .text {
        text-align: justify;
    }
}

.post {
    margin-top: 20px;
    clear: both;

    .pic {
        float: left;
        clear: both;
        margin-right: 20px;
        margin-bottom: 15px;
    }

    .text {
        text-align: justify;
    }
}

I hope it helps! 👍

@JaapJoris JaapJoris added cannot-reproduce and removed help wanted Extra attention is needed labels Dec 16, 2022
@JaapJoris
Copy link
Member

Thank you very much for your help, @golekmichal! Since I got the same result myself, I will now close this issue as cannot-reproduce. Feel free to reopen with a reproducible case 🤗

@GitRon
Copy link
Author

GitRon commented Dec 19, 2022

Super odd that it happens only with my machine. Thx for evaluation!

JaapJoris added a commit that referenced this issue Jan 30, 2023
The main use case of DjHTML is to modify files in-place. Therefore, the `-i`
(`--in-place`) argument has been made the default and removed, leaving only
a deprecation error. The `-o` (`--output-file`) argument has also been
removed.

Reading from standard input and writing to standard output is still
supported by using "-" as the filename. By the same mechanism, writing the
output to a different file is also still possible by using output
redirection:

    djhtml - < input.html > output.html

Since this is a backwards-incompatible change, the next release will
increment the major version number. No changes have been made to the
indentation algorithm.

Closes #66
Closes #69
Closes #70
@JaapJoris
Copy link
Member

In hindsight, I shouldn't have been so eager to close this issue. My apologies. It turned out to be an actual bug in DjHTML, and the only reason me and the people I asked couldn't reproduce it was because we weren't using Windows.

Hopefully, this issue has now been fixed once and for all in the latest release of DjHTML. Could you please let me know whether it works?

@GitRon
Copy link
Author

GitRon commented Feb 1, 2023

@JaapJoris It works! ❤️ Thx for all your effort!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants