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

Jinja formatting issues #715

Merged
merged 7 commits into from
Aug 28, 2024
Merged

Conversation

jessielw
Copy link
Contributor

Pull Request Check List

Resolves: #issue-number-here

  • Added tests for changed code.
  • Updated documentation for changed code.

… whole of the file

- Added code to handle extra spacing inside the double quotes, as well as ensure only single quotes
- It also removes any extra white space based on teh word boundaries
@netlify
Copy link

netlify bot commented Jul 15, 2023

Deploy Preview for djlint ready!

Name Link
🔨 Latest commit 71698c0
🔍 Latest deploy log https://app.netlify.com/sites/djlint/deploys/64f1d2647d2f640008a3b370
😎 Deploy Preview https://deploy-preview-715--djlint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@jessielw
Copy link
Contributor Author

Note: This forces double quotes on the outside and single quotes inside.

…ach section and correctly modify the inner quotes

- This will always prioritize what ever the user had set on the outer quotes and will only activate for jinja templates
@jessielw
Copy link
Contributor Author

jessielw commented Jul 16, 2023

Update: New code is stable, passes all tests, allows the linter to detect the outer most quotes and apply changes to the inner most to prevent vscode and syntax errors as well as handles spacing issues that can be caused by using the built in formatter in vscode and then using djlint

@codecov
Copy link

codecov bot commented Jul 17, 2023

Codecov Report

Patch coverage: 29.62% and project coverage change: -1.69% ⚠️

Comparison is base (07dfbad) 95.35% compared to head (e9dc2aa) 93.67%.
Report is 18 commits behind head on master.

❗ Current head e9dc2aa differs from pull request most recent head 71698c0. Consider uploading reports for the commit 71698c0 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #715      +/-   ##
==========================================
- Coverage   95.35%   93.67%   -1.69%     
==========================================
  Files          16       16              
  Lines        1033     1059      +26     
  Branches      278      285       +7     
==========================================
+ Hits          985      992       +7     
- Misses         34       50      +16     
- Partials       14       17       +3     
Files Changed Coverage Δ
src/djlint/formatter/indent.py 87.19% <29.62%> (-11.36%) ⬇️

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

Copy link
Contributor

@christopherpickering christopherpickering 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 this, I appreciate it. I put a few comments > if you don't want to invest more time you can leave as is and I can pick it up sometime.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool, instead of changing the test here, can you just do 2 tests?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nunjucks is pretty much identical to jinja, and anyone else using quotes can benefit from this :) Do you mind to remove the the profile check?

image

aren't cases like this still going to confuse vs code?

The other odd case is where there is a line break:

image

I think if we move this down to where we do the set tag formatting, it will be able to work w/ wrapping.

At the same time it might be good to add a flag --single-quote-templatetags, vs a --single-quote tag we can add later on for just html attributes.

Thanks for starting this, if you don't wanna invest more time you can leave as is and I can pick it up sometime.

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 can take a look at it to see if I can improve it further in the ways you suggested.

Specifically by set tag formatting are you referring to?
image

In reference to this photo
image
I'm not 100% how we could remedy this yet.

I'll think on some things and take a look at this tomorrow.

Copy link
Contributor Author

@jessielw jessielw Jul 18, 2023

Choose a reason for hiding this comment

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

Ah you must be referring to for the set tag
image

If this is not correct please point me where you think i should impliment thr changes

I'll look into this and see if I can improve it further.

As far as the linter for
<a href='{{ url("fo\"o") }}'></a>
what would you suggest to improve this?

.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey sorry for the delay, yep I think this is the place- it is where the contents of functions are updated.

I'm free to discord call early afternoon today if it's good for you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Feel free to add me on discord jlw_4049 so we can talk quicker

@christopherpickering
Copy link
Contributor

Sorry for the delay here. I updated the test case a bit, and added a more complex test. Can every see if they agree w/ this code:

  • if outer quote is a double, the first set of inner should be a singe. Lines 1-4 didn't make this change
  • notice line 3 and 4 lost a backslash in a string... it should not have been dropped
  • this should probably be a config action, I see issues coming up w/ folks who disagree (at the same time remove the jinja restriction)

image

Copy link

netlify bot commented Feb 12, 2024

Deploy Preview for djlint canceled.

Name Link
🔨 Latest commit 14dc271
🔍 Latest deploy log https://app.netlify.com/sites/djlint/deploys/65c9841a578bd400097352e3

@jessielw
Copy link
Contributor Author

Sorry for the delay here. I updated the test case a bit, and added a more complex test. Can every see if they agree w/ this code:

  • if outer quote is a double, the first set of inner should be a singe. Lines 1-4 didn't make this change
  • notice line 3 and 4 lost a backslash in a string... it should not have been dropped
  • this should probably be a config action, I see issues coming up w/ folks who disagree (at the same time remove the jinja restriction)

image

Not really sure how we could remedy this. VSCode is still going to complain about the expected test output.

Not really sure what the best approach would be to remedy this since VSCode doesn't like the expected results either.
image

@dakixr
Copy link

dakixr commented Aug 24, 2024

Hey, just checking what is missing here before merging?

I am using the ident.py changes locally and seems to be working just fine

@jessielw
Copy link
Contributor Author

The post I made above yours is the last I messed with it. The repository owner isn't maintaining it right now to my understanding. There is @monosans I believe but haven't heard anything on this subject from him. Perhaps he could shed some light on a solution to this.

@monosans monosans changed the base branch from master to dev August 28, 2024 13:08
@monosans monosans changed the base branch from dev to master August 28, 2024 13:09
@monosans monosans merged commit 892dac1 into djlint:master Aug 28, 2024
4 checks passed
@christopherpickering
Copy link
Contributor

🎉 This PR is included in version 1.34.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@dakixr
Copy link

dakixr commented Aug 29, 2024

Hi @christopherpickering, I just updated the package to the latest release, tks for your incredible work!

However this formatting issue is still present.

Just as an example:

code:

<link rel="stylesheet" href="{{ static('css/output.css') }}?{{ now() .strftime('%d-%m-%Y') }}" />

gets formatted into:

<link rel="stylesheet" href="{{ static("css/output.css") }}?{{ now().strftime('%d-%m-%Y') }}" />

As a workaround I am manually changing ident.py with https://github.com/jessielw/djLint/blob/7b7e504de721e51cf4c5dcc8115540145a8eac23/src/djlint/formatter/indent.py
contents.

Is there any reason why this changes were not included in the new releases?

@monosans
Copy link
Member

Hi @christopherpickering, I just updated the package to the latest release, tks for your incredible work!

However this formatting issue is still present.

Just as an example:

code:

<link rel="stylesheet" href="{{ static('css/output.css') }}?{{ now() .strftime('%d-%m-%Y') }}" />

gets formatted into:

<link rel="stylesheet" href="{{ static("css/output.css") }}?{{ now().strftime('%d-%m-%Y') }}" />

As a workaround I am manually changing ident.py with jessielw/djLint@7b7e504/src/djlint/formatter/indent.py contents.

Is there any reason why this changes were not included in the new releases?

Seems like I've merged branches incorrectly. Will take a look right now. FYI Christopher is not maintaining djLint anymore.

@dakixr
Copy link

dakixr commented Aug 29, 2024

Hi @christopherpickering, I just updated the package to the latest release, tks for your incredible work!
However this formatting issue is still present.
Just as an example:
code:

<link rel="stylesheet" href="{{ static('css/output.css') }}?{{ now() .strftime('%d-%m-%Y') }}" />

gets formatted into:

<link rel="stylesheet" href="{{ static("css/output.css") }}?{{ now().strftime('%d-%m-%Y') }}" />

As a workaround I am manually changing ident.py with jessielw/djLint@7b7e504/src/djlint/formatter/indent.py contents.
Is there any reason why this changes were not included in the new releases?

Seems like I've merged branches incorrectly. Will take a look right now. FYI Christopher is not maintaining djLint anymore.

Oh so I will rephrase then, tks you @monosans for your work!

Got it!

@monosans
Copy link
Member

I'm pretty sure I've merged everything correctly. Need to find the reason why it doesn't work.

@dakixr
Copy link

dakixr commented Aug 29, 2024

The issue is with the commit: "- Add the ability to detect what ever the outer most quotes was for e…"

That one re-introduced the bug.

I am using this script to fix it locally for the new releases:

#!/bin/bash

# URL to fetch the new contents from
# Commit: WORKING - "- Added some regex to ensure we have double quoted attributes for the…"
URL="https://raw.githubusercontent.com/jessielw/djLint/7b7e504de721e51cf4c5dcc8115540145a8eac23/src/djlint/formatter/indent.py"
# Commit: BROKEN - "- Add the ability to detect what ever the outer most quotes was for e…"
# URL="https://raw.githubusercontent.com/jessielw/djLint/e9dc2aaf7c977def61af6c75fd0b7fc87634d198/src/djlint/formatter/indent.py"

# Find all indent.py files under **/djlint/formatter/ directories
files=$(find . -type f -path "*/djlint/formatter/indent.py")

# Loop over each found file and replace its contents
for file in $files; 
do
    echo "Replacing contents of $file"
    curl -s $URL -o "$file"
done

echo "Replacement complete."

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

Successfully merging this pull request may close these issues.

4 participants