-
Notifications
You must be signed in to change notification settings - Fork 14.4k
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
test(frontend): use absolute path for src imports #9761
Conversation
41db2fb
to
819a8f9
Compare
@@ -1,4 +1,5 @@ | |||
{ | |||
"singleQuote": true, | |||
"trailingComma": "all" | |||
"trailingComma": "all", | |||
"arrowParens": "avoid" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prettier
changed the default behavior for arrowParent
. We revert it back to the old behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There seems to be another change to space between function
and parentheses.
function() {}
vs function () {}
I do not have strong preference in terms of styling but perhaps we can use the original to reduce amount of changes.
This is the settings from @superset-ui
{
"arrowParens": "avoid",
"bracketSpacing": true,
"jsxBracketSameLine": false,
"printWidth": 100,
"proseWrap": "always",
"requirePragma": false,
"semi": true,
"singleQuote": true,
"tabWidth": 2,
"trailingComma": "all",
"useTabs": false
};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems Prettier folks decided this is somehow the preferred style and didn't add an option to turn it off.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ic. If no strong objection from others I am good with this. I'll approve but wait for others to have a chance to see the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer without the space, but if there's no way to turn it off, it won't break my heart.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just read through some of the threads on that feature, and now I'm glad I don't work on that project. Yeesh!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we merge this then? 😄
f0a08b3
to
492d404
Compare
492d404
to
93aeede
Compare
Codecov Report
@@ Coverage Diff @@
## master #9761 +/- ##
==========================================
- Coverage 70.81% 70.80% -0.02%
==========================================
Files 586 586
Lines 30445 30444 -1
Branches 3121 3120 -1
==========================================
- Hits 21559 21555 -4
- Misses 8772 8775 +3
Partials 114 114
Continue to review full report at Codecov.
|
Jeez... I keep spending forever looking at these things, and then when I'm done, I look up and see |
CATEGORY
SUMMARY
This changes all JS imports in
specs
to use absolute path, which makes the code a little bit easier to read and shall make rearranging the test files easier.Added a
jsconfig.json
for VS Code so that intellisense can still work.Also upgraded
prettier
to fix lint.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A
TEST PLAN
Make sure CI passes and other IDEs also have no problem using JS features.
ADDITIONAL INFORMATION
REVIEWERS
@kristw @rusackas @villebro