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

The big reformat #600

Merged
merged 14 commits into from
Jan 17, 2024
Merged

The big reformat #600

merged 14 commits into from
Jan 17, 2024

Conversation

tymees
Copy link
Member

@tymees tymees commented Jan 12, 2024

This PR reformats the entire codebase using Black and DJLint for python and templates respectively.

I checked every file after reformat; the black reformat went well, no issues.

The DJLint reformat had a couple problems:

  • JS formatting was borked (we already knew this), so I just disabled it. We probably want a backlog issue to enable it
  • Some pages were using invalid HTML that I had to correct; a couple of examples:
    • <ul><p><li>[..]</li></p></ul>. Those p's aren't allowed there
    • <\br> - yeah dunno how this happened
    • Missing closing tags
  • blocktrans without trimmed messed things up BAD. (It effectively disabled formatting, even for a couple of lines after the end tag). So, I added trimmed to every blocktrans.
  • Translations were borked, but that was to be expected. I think I fixed all issues.
  • Update: there was also one 'Django-in-CSS' that DJLint broke... Luckily this one I could just disable in-place instead of globally

Lastly, this PR enabled CI based checks to see if someone forgot to format their code :)

@tymees tymees requested review from EdoStorm96 and miggol and removed request for EdoStorm96 January 12, 2024 14:45
This is totally not a formatting problem I used to check the checker, which I accidentally left in while committing.
Well.... At least we know the CI check works :')
@tymees
Copy link
Member Author

tymees commented Jan 12, 2024

Hmmm, the djlint check seems to be checking.... reads log... DSC 2? Wut...

I'll look at that next week, I'm done

EDIT: Github downloaded DSC into a local folder, so that folder had to be excluded. Everything is green now! :)

Okay, didn't realize there is this one case where we use Django-in-CSS. Luckily it's just the one. Right?
Copy link
Contributor

@EdoStorm96 EdoStorm96 left a comment

Choose a reason for hiding this comment

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

I'd say it all looks good. Impressive work! We might run into tiny things later ... but on first glance it all looks good!

@miggol
Copy link
Contributor

miggol commented Jan 16, 2024

<\br> - yeah dunno how this happened

Haha, lol

Copy link
Contributor

@miggol miggol left a comment

Choose a reason for hiding this comment

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

Only wonky translation I found was the following:

#: reviews/templates/reviews/review_detail_sidebar.html:46
#, python-format
msgid "Reviewronde gestart op <br /> %(date_start)s."
msgstr "eviewing round started on %(date_start)s."

Went looking for other msgstrs that don't start with a capital letter, and it turns out there's only like 10 of those total. They're all good.

I took the liberty of committing translations fixes directly into this branch. But my other comment still needs looking at.

en
volgende
stap>
>") }}" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Somethings's wrong here

Copy link
Member Author

Choose a reason for hiding this comment

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

Interesting... Looks like DJLint has a bug when it comes to the _('[..]') syntax.

Should be fixed now, I moved the translated string to a variable and referenced that in the default filter

@tymees
Copy link
Member Author

tymees commented Jan 16, 2024

Fun fact, FF crashes if you try to search through the diff of the locale file lol

I took the liberty of committing translations fixes directly into this branch. But my other comment still needs looking at.

I'm not seeing any commits?

Copy link
Contributor

@miggol miggol left a comment

Choose a reason for hiding this comment

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

Whoops, forgot to push. It's there now. No other issues.

@tymees tymees merged commit 78a24bd into develop Jan 17, 2024
4 checks passed
@tymees tymees deleted the style/formatting branch January 17, 2024 10:05
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.

3 participants