-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
console: clean up styles for entry and server code #9646
Conversation
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.
Looks awesome! Just had one question and one suggestion.
@@ -150,14 +150,14 @@ module.exports = function (kibana) { | |||
path: '/api/console/api_server', | |||
method: ['GET', 'POST'], | |||
handler: function (req, reply) { | |||
let server = require('./api_server/server'); | |||
let { sense_version, apis } = req.query; | |||
const server = require('./api_server/server'); |
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.
Any reason not to import
this at the top of the file?
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.
Nope! Good catch
strict: off | ||
wrap-iife: off | ||
no-var: off | ||
prefer-const: 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.
When will we remove these linting rules and defer to the ones in the root?
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.
Just like the console-root eslintrc that is deleted in this PR, we can delete the specific lint configs once we've updated the code in those directories (public and api_server).
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.
Sounds good. Can we track these tasks in a linting meta-issue? Unless you're planning to move on them immediately, I'm afraid that we'll lose sight of them.
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.
The linked issue is the issue for tracking this, but I've added the meta label to it and added four todo items, two of which are tackled here.
59dfbed
to
522acae
Compare
I removed that |
@@ -150,14 +151,13 @@ module.exports = function (kibana) { | |||
path: '/api/console/api_server', | |||
method: ['GET', 'POST'], | |||
handler: function (req, reply) { | |||
let server = require('./api_server/server'); | |||
let { sense_version, apis } = req.query; | |||
const { senseVersion, apis } = req.query; |
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.
Originally we expected sense_version
in the response, but now we're expecting senseVersion
. Do we need to update any server-side code to make a corresponding change?
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'm going to change it back since it's pulled from the request.
To be honest, I'm surprised this change worked at all, so I suspect there's some unused code/behaviors here. In any case, that's beyond the scope of this change, so I'd prefer to keep this to purely internal changes.
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.
Reverted it back to sense_version
Much of the code in console is original from when the project was a separate plugin and was not updated when it was ported over. Now, any code in the root of the project or in the server directory will be linted based on the global rules in Kibana.
522acae
to
aa82918
Compare
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.
LGTM!
Thanks! |
Backports PR #9646 **Commit 1:** console: clean up styles for entry and server code Much of the code in console is original from when the project was a separate plugin and was not updated when it was ported over. Now, any code in the root of the project or in the server directory will be linted based on the global rules in Kibana. * Original sha: aa82918 * Authored by Court Ewing <[email protected]> on 2016-12-26T20:02:30Z
Backports PR #9646 **Commit 1:** console: clean up styles for entry and server code Much of the code in console is original from when the project was a separate plugin and was not updated when it was ported over. Now, any code in the root of the project or in the server directory will be linted based on the global rules in Kibana. * Original sha: aa82918 * Authored by Court Ewing <[email protected]> on 2016-12-26T20:02:30Z
Much of the code in console is original from when the project was a
separate plugin and was not updated when it was ported over. Now, any
code in the root of the project or in the server directory will be
linted based on the global rules in Kibana and more closely follow our main
JS styleguide.
For #9603