You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2020/03/23 22:48:04 UTC
[GitHub] [incubator-superset] rusackas opened a new pull request #9356:
linting some LESS
rusackas opened a new pull request #9356: linting some LESS
URL: https://github.com/apache/incubator-superset/pull/9356
### CATEGORY
Choose one
- [ ] Bug Fix
- [ ] Enhancement (new features, refinement)
- [x] Refactor
- [ ] Add tests
- [ ] Build / Development Environment
- [ ] Documentation
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
Just a few freshly linted/prettified LESS files. Not sure how this slipped through the cracks before, but, well... here ya have it :)
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TEST PLAN
<!--- What steps should be taken to verify the changes -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Changes UI
- [ ] Requires DB Migration.
- [ ] Confirm DB Migration upgrade and downgrade tested.
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
### REVIEWERS
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] rusackas merged pull request #9356: Enforcing
linting of LESS
Posted by GitBox <gi...@apache.org>.
rusackas merged pull request #9356: Enforcing linting of LESS
URL: https://github.com/apache/incubator-superset/pull/9356
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] rusackas commented on issue #9356: linting
some LESS
Posted by GitBox <gi...@apache.org>.
rusackas commented on issue #9356: linting some LESS
URL: https://github.com/apache/incubator-superset/pull/9356#issuecomment-603538976
> I saw this too and I was wondering if `stylesheets/fonts/FiraCode/` belonged into a `vendors/` or `external/` folder instead of `stylesheets/`
>
> It depends on whether we're taking ownership of the code/assets and are likely to alter the source, or whether we're more likely to leave it untouched or upgrade in place.
I originally thought to put it into /stylesheets because there IS a stylesheet in there that's referenced. Putting that part in vendors/external didn't make so much sense to me since we have/will make edits to the file(s).
How about in another PR, I move the binaries to some sort of vendor assets folder, and adjust the paths in the fonts' stylesheets? If you'd rather I shoehorn that in here, I certainly can :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] mistercrunch commented on issue #9356: linting
some LESS
Posted by GitBox <gi...@apache.org>.
mistercrunch commented on issue #9356: linting some LESS
URL: https://github.com/apache/incubator-superset/pull/9356#issuecomment-603032697
I saw this too and I was wondering if `stylesheets/fonts/FiraCode/` belonged into a `vendors/` or `external/` folder instead of `stylesheets/`
It depends on whether we're taking ownership of the code/assets and are likely to alter the source, or whether we're more likely to leave it untouched or upgrade in place.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] etr2460 commented on a change in pull request
#9356: Enforcing linting of LESS
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #9356: Enforcing linting of LESS
URL: https://github.com/apache/incubator-superset/pull/9356#discussion_r397569330
##########
File path: superset-frontend/package.json
##########
@@ -16,7 +16,7 @@
"prod": "node --max_old_space_size=4096 ./node_modules/webpack/bin/webpack.js --mode=production --colors --progress",
"build-dev": "cross-env NODE_OPTIONS=--max_old_space_size=8192 NODE_ENV=development webpack --mode=development --colors --progress",
"build": "cross-env NODE_OPTIONS=--max_old_space_size=8192 NODE_ENV=production webpack --mode=production --colors --progress",
- "lint": "eslint --ignore-path=.eslintignore --ext .js,.jsx,.ts,.tsx .",
+ "lint": "eslint --ignore-path=.eslintignore --ext .js,.jsx,.ts,.tsx . && prettier --check '{src,stylesheets}/**/*.{css,less,sass,scss}'",
Review comment:
does eslint enforce the prettier checks already? Maybe we should move them out of eslint and into prettier if we're running the prettier checks anyway?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] codecov-io commented on issue #9356: linting
some LESS
Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #9356: linting some LESS
URL: https://github.com/apache/incubator-superset/pull/9356#issuecomment-602917282
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9356?src=pr&el=h1) Report
> Merging [#9356](https://codecov.io/gh/apache/incubator-superset/pull/9356?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/5d9857544a189939128d296ed963446eff2b2815&el=desc) will **not change** coverage by `%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/incubator-superset/pull/9356?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #9356 +/- ##
=======================================
Coverage 58.97% 58.97%
=======================================
Files 374 374
Lines 12124 12124
Branches 2987 2987
=======================================
Hits 7150 7150
Misses 4795 4795
Partials 179 179
```
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9356?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9356?src=pr&el=footer). Last update [5d98575...c2226cb](https://codecov.io/gh/apache/incubator-superset/pull/9356?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] codecov-io edited a comment on issue #9356:
Enforcing linting of LESS
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #9356: Enforcing linting of LESS
URL: https://github.com/apache/incubator-superset/pull/9356#issuecomment-602917282
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9356?src=pr&el=h1) Report
> Merging [#9356](https://codecov.io/gh/apache/incubator-superset/pull/9356?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/499f9c8fca3ea01b7f7eeb977531669518c5dc4b&el=desc) will **not change** coverage by `%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/incubator-superset/pull/9356?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #9356 +/- ##
=======================================
Coverage 59.01% 59.01%
=======================================
Files 378 378
Lines 12162 12162
Branches 3004 3004
=======================================
Hits 7177 7177
Misses 4805 4805
Partials 180 180
```
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9356?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9356?src=pr&el=footer). Last update [499f9c8...1c56a73](https://codecov.io/gh/apache/incubator-superset/pull/9356?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] rusackas commented on issue #9356: linting
some LESS
Posted by GitBox <gi...@apache.org>.
rusackas commented on issue #9356: linting some LESS
URL: https://github.com/apache/incubator-superset/pull/9356#issuecomment-602905549
> Noticed this too when running the `lint-fix` script.
>
> Proposal:
> adding `prettier --check '{src,stylesheets}/**/*.{css,less,sass,scss}'` to the lint script so CI can check if `lint-fix` needs to be run.
Fantastic idea... I added it to this PR :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] rusackas commented on a change in pull request
#9356: Enforcing linting of LESS
Posted by GitBox <gi...@apache.org>.
rusackas commented on a change in pull request #9356: Enforcing linting of LESS
URL: https://github.com/apache/incubator-superset/pull/9356#discussion_r397618872
##########
File path: superset-frontend/package.json
##########
@@ -16,7 +16,7 @@
"prod": "node --max_old_space_size=4096 ./node_modules/webpack/bin/webpack.js --mode=production --colors --progress",
"build-dev": "cross-env NODE_OPTIONS=--max_old_space_size=8192 NODE_ENV=development webpack --mode=development --colors --progress",
"build": "cross-env NODE_OPTIONS=--max_old_space_size=8192 NODE_ENV=production webpack --mode=production --colors --progress",
- "lint": "eslint --ignore-path=.eslintignore --ext .js,.jsx,.ts,.tsx .",
+ "lint": "eslint --ignore-path=.eslintignore --ext .js,.jsx,.ts,.tsx . && prettier --check '{src,stylesheets}/**/*.{css,less,sass,scss}'",
Review comment:
Eslint Pretties(?) and checks the Prettification(?!) of of JS/JSX/TS/TSX files. Prettier itself is being used directly on LESS/CSS files a la carte. So now this verifies them in much the same way.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] rusackas commented on issue #9356: Enforcing
linting of LESS
Posted by GitBox <gi...@apache.org>.
rusackas commented on issue #9356: Enforcing linting of LESS
URL: https://github.com/apache/incubator-superset/pull/9356#issuecomment-607627195
> I saw this too and I was wondering if `stylesheets/fonts/FiraCode/` belonged into a `vendors/` or `external/` folder instead of `stylesheets/`
>
> It depends on whether we're taking ownership of the code/assets and are likely to alter the source, or whether we're more likely to leave it untouched or upgrade in place.
Went with /fonts just to be a little more explicit.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] rusackas commented on issue #9356: Enforcing
linting of LESS
Posted by GitBox <gi...@apache.org>.
rusackas commented on issue #9356: Enforcing linting of LESS
URL: https://github.com/apache/incubator-superset/pull/9356#issuecomment-605364854
@mistercrunch I moved the font binaries to a `/fonts` folder, and shuffled the LESS files around accordingly. Let me know if that suffices. :D
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org