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

Pre-commit (prettier + black) #73

Merged
merged 10 commits into from
May 25, 2023

Conversation

VeckoTheGecko
Copy link
Contributor

I thought auto-formatters would help with maintaining the site.

  • Added prettier (formats HTML, CSS, JS, YAML, JSON, etc) and black (formats Python) using pre-commit
  • Fixed* HTML
  • Ran pre-commit

Maintainer TODO:

  • add pre-commit ci bot to this repo

Feel free to reject this PR if change if too invasive.

*index.html has many unclosed tags which I couldn't fix. Added to .prettier-ignore for now so that pre-commit doesn't error out.

Added black and prettier
@VeckoTheGecko VeckoTheGecko marked this pull request as draft May 21, 2023 13:43
@VeckoTheGecko
Copy link
Contributor Author

VeckoTheGecko commented May 21, 2023

Accidentally introduced breaking changes to index.html with my prior html fixes (dd7f3d9). Reworking to leave index.html untouched.

@VeckoTheGecko VeckoTheGecko marked this pull request as ready for review May 21, 2023 14:40
@erikvansebille
Copy link
Member

Thanks for starting this PR, @VeckoTheGecko. While it looks nice for index.html (and I guess we do need to refactor that; it's gotten a bit out of control!), I'm not sure about all the changes in the other files (e.g. the json files for the drifter data). This red has become somewhat of a catch-all for when we need a lace to put material/data online, and all the reformatting might unintentionally break workflows downstream.

Also, quite a few files are legacy, e.g. from older conferences such as the vegu21. Not sure they need updating either..

So please don't touch files other than the core ones for the website, in the refactoring?

@VeckoTheGecko
Copy link
Contributor Author

Can do, didn't realise that json data files were in the repo.

Switched the prettier ignore file to an allowlist instead, looking at:

  • utrechtteam.html
  • faq.html

Much smaller diff this time 😄

@erikvansebille
Copy link
Member

OK, this looks much better! But now index.html (arguably the most important file) is not updated at all? I see you commented it out of the prettier ignore file; why is it broken?

Read in index.html using beautiful soup, and write out. This automatically fixes some errors like unclosed tags.

```py
from bs4 import BeautifulSoup
html_file = "index.html"
with open(html_file, 'r') as file:
    html_content = file.read()
soup = BeautifulSoup(html_content, 'html.parser')
with open(html_file, 'w', encoding="utf-8") as file:
    file.write(str(soup))
```
Side by side comparison has been used to ensure that these manual changes don't visually change the website.
@VeckoTheGecko
Copy link
Contributor Author

There were quite a few errors within the file (see https://validator.w3.org/nu/?doc=https%3A%2F%2Foceanparcels.org%2F) including tags that weren't closed, and incorrect nesting of elements (e.g. divs in p tags, which aren't permitted and close the p tag early). These errors in particular broke prettier. I was having difficulty hunting these down due to the size of the file and the tooling I was using.

Managed to fix these now I think (with the caveat that the tooling I used in b8301be made a nasty diff making it difficult to see where the parser inserted the missing closing tags). 09daaba used prettier to catch remaining errors (some where beautiful soup misplaced 2 closing tags).

Comparing the final page to the current website, they are visually indistinguishable to me. So I guess the parsing done in b8301be is the same as how web-browsers parse broken HTML.

This is definitely a messy PR with messy diffs. I'm not even sure if doing these fixes to introduce formatting helps from a maintainence POV, or is just noise. Feel free to close this if its more headache than its worth (no hard feelings from this end) 🙂.

Feel free to specify any prettier config options you'd like in a .prettierrc json config file to customise indentation, max width, etc etc (I saw the index.html file ballooned from 3500 to 9000 lines; partially caused by the hard text wrapping introduced by prettier). If you want, you can let me know the options, and I can cherry pick this PR to ensure that all the files are ran from master with the proper options (should only take a few minutes).

Copy link
Member

@erikvansebille erikvansebille left a comment

Choose a reason for hiding this comment

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

Changes look good! Just one minor comment. And then I guess we (I?) need to set up something local for the pre-commit to work?

.prettierignore Outdated Show resolved Hide resolved
@VeckoTheGecko
Copy link
Contributor Author

VeckoTheGecko commented May 25, 2023

Either in CI (pre-commit ci which is free for open source projects), or local through brew, pypi, or conda (installation). Local setup is just a matter of installing then running pre-commit install to install the hooks such that they are run prior to committing. I normally run both local and CI for my projects (I mainly use local so that things are done prior to committing, where CI will actually run on PRs and push patches to the branch).

@erikvansebille erikvansebille merged commit d555394 into OceanParcels:master May 25, 2023
@VeckoTheGecko VeckoTheGecko deleted the feat/pre-commit2 branch May 25, 2023 08:50
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