search

B612-Asteroid-Institute / precovery

Fast precovery of small body observations at scale
BSD 3-Clause "New" or "Revised" License
6 stars 2 forks source link

Update pre-commit #59

Closed spenczar closed 1 year ago

spenczar commented 1 year ago

Updated pre-commit to use a modern version of black, since the old pinned version has a version compatibility issue with the latest version of click.

Removed pip-tools from pre-commit since it appears they are no longer used.

Ran pre-commit run --all-files, which made a lot of whitespace changes.

moeyensj commented 1 year ago

One minor issue for Markdown files is that two whitespaces at the end of a line are interpreted as wanting a line break so if we clear trailing whitespaces there we lose formatting capabilities. I've set my vscode to ignore removing whitespaces in *.md files on save.

spenczar commented 1 year ago

One minor issue for Markdown files is that two whitespaces at the end of a line are interpreted as wanting a line break so if we clear trailing whitespaces there we lose formatting capabilities. I've set my vscode to ignore removing whitespaces in *.md files on save.

Whoa, that's news to me. Very strange.

Do we actually lose formatting capabilities, or we'd need to format line breaks with actual line breaks?

spenczar commented 1 year ago

https://github.com/pre-commit/pre-commit-hooks/pull/58 appears to fix this. I'll make a change to the config (possibly tomorrow) to specifically ignore markdown soft breaks.

moeyensj commented 1 year ago

One minor issue for Markdown files is that two whitespaces at the end of a line are interpreted as wanting a line break so if we clear trailing whitespaces there we lose formatting capabilities. I've set my vscode to ignore removing whitespaces in *.md files on save.

Whoa, that's news to me. Very strange.

Do we actually lose formatting capabilities, or we'd need to format line breaks with actual line breaks?

We could swap to using <br> with a caveat. I've had this debate in my mind before and ended up consulting: https://www.markdownguide.org/basic-syntax/#line-break-best-practices Unfortunately, not all Markdown applications support HTML tags though.

moeyensj commented 1 year ago

pre-commit/pre-commit-hooks#58 appears to fix this. I'll make a change to the config (possibly tomorrow) to specifically ignore markdown soft breaks.

Nice find! Yeah, this would do it.

spenczar commented 1 year ago

@moeyensj Okay, fixed it, and restored the old softbreaks.

moeyensj commented 1 year ago

Regarding the actual purpose of this PR: I'm more than happy to adopt pre-commit. The main reason we dropped it is as you suspected: you were the only one using it. If we agree on the formatting org-wide then I'm in favor of adding pre-commit as a development dependency since it will make our contributions easier to parse. We will want to update our developer guide with the relevant details if we decide to merge.

I'm leaving that decision to our head of software engineering!

spenczar commented 1 year ago

@akoumjian I've added README.md instructions. I've also written a little wiki page here: https://github.com/B612-Asteroid-Institute/developer-wiki/wiki/Using-pre-commit

I can merge that into the Developer Guide on the wiki if you think it looks good.

spenczar commented 1 year ago

Linting failures are fixed in https://github.com/B612-Asteroid-Institute/precovery/pull/61.