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.

Sign up for GitHub

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

Jump to bottom

Add id-length ESLint rule #1982

Closed
wants to merge 1 commit into from
Closed

Add id-length ESLint rule #1982

wants to merge 1 commit into from

Conversation

ehmicky
Copy link
Contributor

@ehmicky ehmicky commented Oct 15, 2020 •
edited
Loading

This adds the id-length ESLint rule.

Even with autocompletion, long variables can be annoying. On the other hand, too short variables can be obscure. So this PR gives a 24 characters maximum per variable, which is hopefully a good balance. Please let me know if this should be decreased or increased.

@ehmicky ehmicky added the type: chore work needed to keep the product and development running smoothly label Oct 15, 2020
@ehmicky ehmicky requested a review from erezrokah October 15, 2020 20:58
@ehmicky ehmicky self-assigned this Oct 15, 2020
ehmicky
ehmicky commented Oct 15, 2020
View reviewed changes
packages/build/src/core/feature_flags.js Outdated
@@ -12,12 +12,14 @@ const getFeatureFlag = function (name) {
return { [name]: true }
}

/* eslint-disable id-length */
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Feature flags are often long, and that's ok.

ehmicky
ehmicky commented Oct 15, 2020
View reviewed changes
packages/build/tests/env/fixtures/gatsby_telemetry/plugin.js Outdated
@@ -1,4 +1,5 @@
const {
// eslint-disable-next-line id-length
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an external environment variable.

@ehmicky ehmicky mentioned this pull request Oct 16, 2020
Merged
erezrokah
erezrokah suggested changes Oct 20, 2020
View reviewed changes
Copy link
Contributor

@erezrokah erezrokah left a comment

Choose a reason for hiding this comment

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

Didn't go over all the changes, since I wanted to start a discussion.
Min length makes sense, but max length can be too restricting.
I added some comments where the previous version was more explicit and readable (to me).
Please let me know what you think

packages/build/src/core/constants.js Outdated
@@ -82,12 +82,12 @@ const getEdgeHandlersSrc = async function (edgeHandlers, buildDir) {
return edgeHandlers
}

if (await isDirectory(`${buildDir}/${DEFAULT_EDGE_HANDLERS_SRC}`)) {
return DEFAULT_EDGE_HANDLERS_SRC
if (await isDirectory(`${buildDir}/${DEFAULT_HANDLERS_SRC}`)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The longer name is more explicit here and I think it is better, WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

packages/build/src/install/functions.js Outdated
@@ -2,18 +2,18 @@ const { dirname } = require('path')

const readdirp = require('readdirp')

const { logInstallFunctionDependencies } = require('../log/messages/install')
const { logInstallFunctionDeps } = require('../log/messages/install')
Copy link
Contributor

Choose a reason for hiding this comment

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

[sand] I prefer the non abbreviated logInstallFunctionDependencies name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

packages/build/src/log/messages/install.js Outdated
@@ -17,13 +17,13 @@ const logMissingPluginsWarning = function (logs, packages) {
logArray(logs, packages, { color: THEME.errorSubHeader })
}

const logInstallLocalPluginsDeps = function (logs, localPluginsOptions) {
const logInstallLocalDeps = function (logs, localPluginsOptions) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the longer name makes sense here, WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@ehmicky
Copy link
Contributor Author

ehmicky commented Oct 20, 2020 •
edited
Loading

This makes sense 👍
I personally prefer to keep variables not too long, but this is definitely more of a personal preference, and there are some very valid reasons to keep some variables long as you are mentioning.

What would you prefer?

  1. Removing any constraint on the max length?
  2. Keeping the max length constraint, but with a higher value (in which case, please let me know which would be best)?

Both solutions sound good to me 👍

@erezrokah
Copy link
Contributor

erezrokah commented Oct 21, 2020 •
edited
Loading

  1. Removing any constraint on the max length?

I would go with this one, haven't seen many cases with a very long name that wasn't justified.

@ehmicky ehmicky force-pushed the chore/lint-id-length branch from 1ee8dc3 to 9afcc3f Compare October 21, 2020 12:35
@ehmicky
Copy link
Contributor Author

ehmicky commented Oct 21, 2020 •
edited
Loading

Sounds good, I have removed the max constraint.
I also removed all the changes in this PR related to long variable names, so this PR is now much smaller.

@ehmicky ehmicky requested review from erezrokah and removed request for erezrokah October 21, 2020 12:36
@ehmicky
Copy link
Contributor Author

ehmicky commented Oct 26, 2020

Done as part of #2004.

@ehmicky ehmicky closed this Oct 26, 2020
@ehmicky ehmicky deleted the chore/lint-id-length branch October 26, 2020 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@erezrokah erezrokah Awaiting requested review from erezrokah

Assignees

@ehmicky ehmicky

Labels
type: chore work needed to keep the product and development running smoothly
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ehmicky @erezrokah