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

Add prettier #1687

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

janschoenherr
Copy link
Contributor

This can be run with npx prettier . --check or npx prettier . --write

It will currently only check the ./src folder.

@hansmorb
Copy link
Contributor

hansmorb commented Dec 8, 2024

Hält sich das denn an in der .editorconfig definierten Codestyle?

@datengraben
Copy link
Contributor

@janschoenherr From my perspective you can add the includes and tests folder also.

  • Question: What are you thoughts on configuring it only for php or for all of html/js/php?
    The template files php and js maybe incompatible with getting formatted.

  • Question: What @janschoenherr and @hansmorb think if we add in the next step the git-blame ignore list.
    Github Docs

    Because in the next step we are going to commit big changes into the log that are only cosmetic refactorings. They will pollute the git-blame view. A file with the commits that are cosmetic nature, seems to be a good practice. gutenberg Repo

    But that does not have to be part of this PR.

@datengraben datengraben mentioned this pull request Dec 10, 2024
@janschoenherr
Copy link
Contributor Author

janschoenherr commented Dec 10, 2024

We are using it for JavaScript, JSON, etc. however we did have problems in the past with php template files (html and php in one file). I can't recall specifics though. So we might want to try those too.

@hansmorb We would probably have to adjust the .editorconfig file to match the behavior. Personally, I am not sure you need so many fine grained settings in that file, if you are combining it with prettier.

Regarding the git-blame ignore list. I didn't know about that :-) Seems to make sense! As you said, I would separate the actual prettier run from this PR though.

I updated the PR to include /includes and /tests/php folders.

Copy link

codecov bot commented Dec 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 54.08%. Comparing base (68715ef) to head (4df470f).

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #1687      +/-   ##
============================================
- Coverage     54.09%   54.08%   -0.01%     
  Complexity     2628     2628              
============================================
  Files            98       98              
  Lines         11655    11655              
============================================
- Hits           6305     6304       -1     
- Misses         5350     5351       +1     

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

@hansmorb
Copy link
Contributor

Du hast doch auch PHPStorm, entspricht dass, was Prettier aus dem Code machen will dem in der .editorconfig definierten Codestyle? Wenn ja passt das. Wenn nein müssen wir nochmal überlegen, da wir uns eigentlich geeinigt hatten uns an diesen Codestyle zu halten.

@datengraben datengraben mentioned this pull request Jan 11, 2025
@datengraben
Copy link
Contributor

@janschoenherr sorry to keep you wating. we updated the repo php code with the already configured phpcbf (formatter). We still would be grateful if you update this PR to only include the js/scss files and don't include any of the php sources anymore.

The php code base got formatted for directories: src, includes and templates and tests.

JS and SCSS code resides in assets directory and is bit scattered.
Starting from the Gruntfile.js, you should be able to find all source files for Javascript and Sass so that Grunt-dest files are not reformatted.

From my understanding, the gruntfile uses the following js sources:

  • assets/public/js/src/**/*.js
  • assets/admin/js/src/*.js

And following sass sources:

  • assets/public/sass/*
  • assets/admin/sass/*

I am unsure if we will kee the assets/public/sass/themes folder (see #1758), but maybe include it anyway.

But the above stated source directories should be enough for an initial prettier config, that formats js and sass sources of the commonsbooking repo.

@hansmorb what do you think?

@datengraben
Copy link
Contributor

datengraben commented Feb 6, 2025

@hansmorb @janschoenherr I think we can include also the tests/cypress folder for prettier formatting, the only JS tests that we have atm.

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