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

Configure prettier pre-commit hook and reformat existing files #97

Merged
merged 1 commit into from
Oct 23, 2023

Conversation

kenibrewer
Copy link
Contributor

@kenibrewer kenibrewer commented Oct 17, 2023

PR Checklist

  • A description of the changes is added to the description of this PR.
  • If there is a related issue, make sure it is linked to this PR.
  • If you've fixed a bug or added code that should be tested, add tests!
  • Documentation in docs is updated

Description of changes

This PR is intended to be merged after #95 . It addresses #96.

It includes the following changes:

  • Adds prettier pre-commit hook for opinionated formatting of markdown
  • Adds an editor-config file to standardize line lengths and indentation style across differing tools
  • README.rst in template folder converted to README.md
  • All existing markdown adjusted to have a single sentence per line (for easier diffs)
  • All existing markdown files reformatted with prettier

@kenibrewer kenibrewer changed the title Configure prettier pre-commit hook and Configure prettier pre-commit hook and reformat existing files Oct 17, 2023
@codecov
Copy link

codecov bot commented Oct 18, 2023

Codecov Report

Merging #97 (dd1b52e) into main (f853ca3) will not change coverage.
Report is 1 commits behind head on main.
The diff coverage is n/a.

@@          Coverage Diff          @@
##            main     #97   +/-   ##
=====================================
  Coverage   0.00%   0.00%           
=====================================
  Files          1       1           
  Lines          5       5           
=====================================
  Misses         5       5           

@fpgmaas
Copy link
Owner

fpgmaas commented Oct 18, 2023

Thanks for the contribution @kenibrewer, it looks promising! If it's okay with you, I'll review this one after we have finalized the PR regarding devcontainers to reduce the diff a bit. I just placed one more comment on that PR, I think we can merge after we have handled that!

@kenibrewer kenibrewer marked this pull request as ready for review October 19, 2023 21:27
@kenibrewer
Copy link
Contributor Author

@fpgmaas This is now ready for review. Two of the checks failed, but they succeeded when I triggered them again by creating a pull request against my fork. I think the issue is probably something due to cache not being invalidated when I force-pushed a rebase to my branch.

@kenibrewer
Copy link
Contributor Author

@fpgmaas When do you think you'll have a chance to review this PR? I've got some docs changes/improvements I'd like to submit, but the diffs would be pretty complicated if I tried to submit them without this one merged into main.

@fpgmaas
Copy link
Owner

fpgmaas commented Oct 23, 2023

@fpgmaas When do you think you'll have a chance to review this PR? I've got some docs changes/improvements I'd like to submit, but the diffs would be pretty complicated if I tried to submit them without this one merged into main.

Hi @kenibrewer, sorry for my relatively slow response! Pull request looks good to me. Am I correct in understanding that the issue. outlined here should not be a problem since we only format indentations and line lengths, and thus there are no issues that can pop-up in our current Jinja templates once they are parsed?

@kenibrewer
Copy link
Contributor Author

@fpgmaas Correct. Prettier is similar to black and it only formats code in ways that doesn't change the functionality. Beyond that, the prettier pre-commit hook for cookecutter-poetry is instructed to ignore the template directory entirely.

@fpgmaas fpgmaas merged commit 41d853b into fpgmaas:main Oct 23, 2023
8 checks passed
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