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

feature/prettier (css, html, js, json, less, md, scss, yaml) #11

Merged
merged 7 commits into from
Jul 6, 2020
Merged

feature/prettier (css, html, js, json, less, md, scss, yaml) #11

merged 7 commits into from
Jul 6, 2020

Conversation

wookiefriseur
Copy link
Contributor

@wookiefriseur wookiefriseur commented Jul 2, 2020

Only tested manually, but works so far.

Adds support for:
css, html, javascript, json, less, markdown, scss, yaml

See #9

Notes:

  1. Changed the order of copy and chown instructions in the dockerfiles so that working dir and write permissions are already set, when yarn install is executed (container doesn't know pushd and popd commands, which would have been an alternative)
  2. Prettier plugins raised execution times to 500-1500 ms even with repeated requests, which is not OK. The official plugins add ∅ 20ms for each plugin, the inofficial ones about 10 times as much. No idea if that is due to how Prettier loads plugins, or if the plugins are not well optimised.

* install node + yarn
* add ignores
* add package.json

Signed-off-by: Matthäus Falkowski <[email protected]>
* implemented css, html, js, json, less, scss, md, yaml
* basic formatting rules for added languages
* remove all prettier plugins (was unacceptably slow)
* implemented: css, html, js, json, less, scss, md, yaml
* basic formatting rules for added languages
* remove all prettier plugins (was unacceptably slow)
* specify prettier version in lock
* change production docker file
@wookiefriseur wookiefriseur mentioned this pull request Jul 2, 2020
14 tasks
@wookiefriseur wookiefriseur changed the title Feature/prettier feature/prettier (css, html, js, json, less, md, scss, yaml) Jul 4, 2020
Copy link
Member

@jr-cologne jr-cologne left a comment

Choose a reason for hiding this comment

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

Thank you very much for your work, @wookiefriseur!

All in all, your PR looks very good and everything seems to work flawlessly.
It's perfectly fine to drop those prettier plugins for additional languages.
If Prettier is not performing well enough, we can easily support other languages like php with other formatters.

As you can see, I have just requested some small changes which have to do with my preference for npm over yarn.
If you don't have any good reasons for using yarn, I would ask you to make the switch to npm.
In my view, installing yarn just to install the prettier package is a little bit pointless.

Moreover, I would be happy if we could also support the sass syntax/language in addition to scss.
Do you know whether Prettier also supports that?

Thank you in advance.
Have a nice evening! 😃

.docker/web/Dockerfile Outdated Show resolved Hide resolved
.docker/web/Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
app/CodeFormatter/PrettierCodeFormatter.php Outdated Show resolved Hide resolved
@jr-cologne jr-cologne added the enhancement New feature or request label Jul 5, 2020
@wookiefriseur
Copy link
Contributor Author

Moreover, I would be happy if we could also support the sass syntax/language in addition to scss.
Do you know whether Prettier also supports that?

It can do sass using the scss parser. I avoided it because it seems to have some problems with semicolons. But it's getting fixes on a regular basis, so I'll just add it and hope for the best.

@jr-cologne
Copy link
Member

It can do sass using the scss parser. I avoided it because it seems to have some problems with semicolons. But it's getting fixes on a regular basis, so I'll just add it and hope for the best.

Alright, let's hope for the best. In case there are too many bugs, we can still drop it in the future. 😃

@wookiefriseur
Copy link
Contributor Author

Alright, let's hope for the best. In case there are too many bugs, we can still drop it in the future. 😃

After testing a bit it looks like the scss parser does not even understand Sass. Will try to find a solution at a later date.

jr-cologne
jr-cologne previously approved these changes Jul 6, 2020
Copy link
Member

@jr-cologne jr-cologne left a comment

Choose a reason for hiding this comment

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

Ok, no problem.
Feel free to merge. :shipit:

jr-cologne
jr-cologne previously approved these changes Jul 6, 2020
@wookiefriseur wookiefriseur mentioned this pull request Jul 6, 2020
@wookiefriseur wookiefriseur merged commit b516263 into dev-community-de:master Jul 6, 2020
@wookiefriseur wookiefriseur deleted the feature/prettier branch July 6, 2020 11:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants