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

Prettier on scss and css files generated + reformat css.ejs and scss.ejs #7476

Merged
merged 4 commits into from Apr 14, 2018

Conversation

JulioJu
Copy link
Contributor

@JulioJu JulioJu commented Apr 12, 2018

  • Please make sure the below checklist is followed for Pull Requests.

Fix #7451

  • Travis tests are green
  • Tests are added where necessary
  • Documentation is added/updated where necessary
  • Coding Rules & Commit Guidelines as per our CONTRIBUTING.md document are followed

@deepu105
Copy link
Member

@JulioJu great work.

options:
jsxBracketSameLine: false
arrowParens: avoid
parser: typescript
Copy link
Contributor Author

@JulioJu JulioJu Apr 12, 2018

Choose a reason for hiding this comment

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

This line is not mandatory. Prettier detects the parser needed.
@gmarziou. To simplify prettierrc, we could delete all "parser" lines? What do you think about that? I asked me before commit, but I didn't dare delete that…
Maybe it's better to have a more accurate prettierc? We have a complete syntax… Maybe it's better ^^! I believe it's better to see what we do… As you wish ;-).

Note: parser will be renamed "language" (see prettier/prettier#2846).

EDIT excuse-me: the original author of this file is @deepu105 ;-)

Copy link
Member

Choose a reason for hiding this comment

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

Lets leave it like that as its more explicit

Copy link
Contributor Author

@JulioJu JulioJu Apr 13, 2018

Choose a reason for hiding this comment

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

@deepu105. I'm pretty sure, this syntax doesn't work. We could deduce that beccause we could see than 7fcd4b0 works, but 7a85416 fail. My assumption is that somewhere (but I don't know where) a command $ yarn prettier --write --parser css "src/**/*.ts" is launched. In my computer, in folder ngx-default-sample, when I run command $ yarn prettier --write --parser css "src/**/*.ts", I have the same kind of error than in line 1526 of https://travis-ci.org/jhipster/generator-jhipster/jobs/365956929 (build of 7a85416).

We could read at line 1526 of https://travis-ci.org/jhipster/generator-jhipster/jobs/365956929 than

SyntaxError: (postcss) CssSyntaxError Unknown word (1:1)
> 1 | export * from './foo.service';
    | ^
  2 | export * from './foo-update.component';
  3 | export * from './foo-delete-dialog.component';

I don't understand this error beacause in folder "ngx-default-sample" there isn't file "foo.service", there isn't entity "foo". Have you an ideay why on remote Travis it tests folder "foo"?

As 7fcd4b0 works, I could --hard reset to this commit ?

Or maybe I could try simply with a .prettierrc without explicit parser? Thanks 7fcd4b0, and with the result of the command « $ yarn prettier --write --parser css "src/**/*.ts » I believe it's better to let prettier choose the parser corresponding to the file extension.

printWidth: 140
singleQuote: true
tabWidth: 4
useTabs: false
overrides:
- files: "src/**/{*.ts,*.tsx}"
  options:
    jsxBracketSameLine: false
    arrowParens: avoid

What do you think about that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@deepu105 I'm going to test the syntax above with a new commit…

jsxBracketSameLine: false
arrowParens: avoid
parser: typescript
- files: "src/**/*.css"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gmarziou : same as above: should we delete it ?

@JulioJu JulioJu changed the title Prettier on scss and css files generated + reformat css.ejs and scss.ejs + Prettier on scss and css files generated + reformat css.ejs and scss.ejs Apr 12, 2018
Copy link
Contributor Author

@JulioJu JulioJu left a comment

Choose a reason for hiding this comment

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

@@ -1,5 +1,5 @@
#!/bin/bash
set -e
set -ex
Copy link
Contributor Author

@JulioJu JulioJu Apr 12, 2018

Choose a reason for hiding this comment

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

There is a strange error so I enable the bash option « trace for each command after it expands the command and before it executes it » to see accurately where there is a bug. Advise at https://docs.travis-ci.com/user/installing-dependencies/#Installing-Projects-from-Source.

Note: when I run $ ./build-samples.sh build ngx-default or yarn prettier:format on my computer, all work fine. Strange!

EDIT:
$ ./build-samples.sh build ngx-default could success because it doesn't call the script who failed: scripts/00-install-jhipster.sh! Moreover, there isn't yarn link, so we dowload each time the last release of JHipster. I will submit a new issue, and maybe a Pull Request.

Copy link
Member

Choose a reason for hiding this comment

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

@pascalgrimaud is this ok?

@jdubois
Copy link
Member

jdubois commented Apr 12, 2018

This is totally awesome!!! Only downside is that @deepu105 you need to modify your Devoxx slides to include this :-D

@JulioJu JulioJu mentioned this pull request Apr 12, 2018
1 task
@JulioJu JulioJu force-pushed the prettier-on-css-and-scss branch 4 times, most recently from a12468d to 7a85416 Compare April 13, 2018 05:50
@JulioJu
Copy link
Contributor Author

JulioJu commented Apr 13, 2018

@deepu105 I'm going to test the idea explained at #7476 (comment).

@JulioJu
Copy link
Contributor Author

JulioJu commented Apr 13, 2018

@deepu105 it works!
I prefer 7fcd4b0 than 3625f47 because 7fcd4b0 it's more clear and maybe more reliable and more human readable.

There is a strange behaviour in the Travis build of 7a85416. I've let an hypothesis, but it's strange. I believe it's better to let Prettier choose which rules and which parser work on a filetype. Be too much accurate in .prettierrc seems introduce confusion during Travis build. Only let a comment for humans is I believe better, and let Prettier works freely is I believe more reliable.

@@ -1071,12 +1074,12 @@ module.exports = class extends Generator {
*/
registerClientTransforms(generator = this) {
if (!generator.skipClient) {
const typescriptFilter = filter(['**/*.{ts,tsx}'], { restore: true });
const myFilter = filter(['src/**/*.{ts,tsx,scss,css}'], { restore: true });
Copy link
Member

Choose a reason for hiding this comment

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

Rename to prettierFilter

@@ -1,5 +1,5 @@
#!/bin/bash
set -e
set -ex
Copy link
Member

Choose a reason for hiding this comment

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

@pascalgrimaud is this ok?

@deepu105
Copy link
Member

@JulioJu ok for your last comment, let prettier choose options, if that works fine. Can you cleanup your commits, may be squash the reverts

@JulioJu
Copy link
Contributor Author

JulioJu commented Apr 13, 2018

@deepu105. Ok squashed ! Commit with set -ex deleted because this afternoon I will work on a separate PR for that (see issue #7479).

@JulioJu JulioJu force-pushed the prettier-on-css-and-scss branch 2 times, most recently from 72ed276 to e19617a Compare April 13, 2018 11:50
@JulioJu
Copy link
Contributor Author

JulioJu commented Apr 13, 2018

@deepu105
I havn't luck with Travis Builds ;-)
Network connection problem again:
«
[FATAL] Non-resolvable parent POM for io.github.jhipster:jhipster-dependencies:[unknown-version]: Failure to find io.github.jhipster:jhipster-parent:pom:2.0.0 in http://repo.maven.apache.org/maven2 was cached in the local repository, resolution will not be reattempted until the update interval of central has elapsed or updates are forced @ io.github.jhipster:jhipster-dependencies:[unknown-version], /home/travis/.m2/repository/io/github/jhipster/jhipster-dependencies/2.0.0/jhipster-dependencies-2.0.0.pom, line 5, column 13
» (https://travis-ci.org/jhipster/generator-jhipster/jobs/366048640).
So I start again the Travis build

@jdubois
Copy link
Member

jdubois commented Apr 13, 2018

No the jhipster-parent failure is my fault!!

@JulioJu
Copy link
Contributor Author

JulioJu commented Apr 13, 2018

@jdubois ok :-) !
So could you restart the travis build when all will be ok ? We could see at https://travis-ci.org/jhipster/generator-jhipster/builds/366077749?utm_source=github_status&utm_medium=notification that all Travis build won't success ;-)

What is « jhipster-parent » ?

@jdubois
Copy link
Member

jdubois commented Apr 13, 2018

"jhipster-parent" is a new library on which "jhipster-framework" and "jhipster-dependencies" depend - they should all be served from Maven Central, and this is what I failed this morning.
I just released version 2.0.1 and upgraded the generator, see f12c59c - it should be good, but it's not fully tested yet. I'm monitoring this right now.

@deepu105 deepu105 merged commit 9ed5e49 into jhipster:master Apr 14, 2018
@JulioJu
Copy link
Contributor Author

JulioJu commented Apr 14, 2018

@jdubois thanks for your explanation :-) ! All work fine now ;-).
@deepu105 thanks for the review and the merge :-).

@jdubois jdubois added this to the 5.0.0-beta.1 milestone May 3, 2018
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.

None yet

3 participants