Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Update babel-eslint to 8.1.1 #2255

Merged
merged 5 commits into from
Oct 29, 2018
Merged

Update babel-eslint to 8.1.1 #2255

merged 5 commits into from
Oct 29, 2018

Conversation

aaronraimist
Copy link
Contributor

@aaronraimist
Update babel-eslint to 8.1.1
790ecbc 
Signed-off-by: Aaron Raimist <aaron@raim.ist>
@aaronraimist aaronraimist mentioned this pull request Oct 27, 2018
Merged
@turt2live
Copy link
Member

Surely this requires a change to the package lock too?

@aaronraimist
Copy link
Contributor Author

It... should? but nothing changed in it

@turt2live
Copy link
Member

weiiird. Alright then. assuming the build passes this lgtm

@aaronraimist
Copy link
Contributor Author

Actually I know why nothing changed 🤦‍♂️. package-lock.json isn't committed in this repo. element-hq/element-web#7083

@turt2live
Copy link
Member

Oh, that would do it too.

@turt2live
Copy link
Member

Looks like upgrading eslint caused lint failures

@aaronraimist
Copy link
Contributor Author

aaronraimist commented Oct 27, 2018 •
edited
Loading

Updating to

  • babel-eslint 10.0.1
  • eslant 5.8.0
  • eslint-plugin-babel 5.2.1

brings it down to 10 errors (which actually look reasonable)

-> sleep and I'll fix this up tomorrow

@aaronraimist
Update more eslint related packages
952bdba 
Signed-off-by: Aaron Raimist <aaron@raim.ist>
@aaronraimist
eslint --fix src/
49ce4ef 
Signed-off-by: Aaron Raimist <aaron@raim.ist>
@aaronraimist
Fix a few no-useless-escape lint errors
5f3b03c 
Signed-off-by: Aaron Raimist <aaron@raim.ist>
@aaronraimist
Copy link
Contributor Author

aaronraimist commented Oct 27, 2018 •
edited
Loading

Never mind I'll just fix it now. When reviewing this pay special attention to 5f3b03c. I think this is right but double check.

See eslint/eslint#7656

@aaronraimist
Fix eslint --fix
e61e7f2 
Signed-off-by: Aaron Raimist <aaron@raim.ist>
@aaronraimist
Copy link
Contributor Author

Sorry it is so massive, I don't think there's a way to eslint --fix errors only

turt2live
turt2live reviewed Oct 27, 2018
View reviewed changes
src/VelocityBounce.js Outdated Show resolved Hide resolved
src/components/views/dialogs/CreateGroupDialog.js
@@ -57,7 +57,7 @@ export default React.createClass({
let error = null;
if (!this.state.groupId) {
error = _t("Community IDs cannot be empty.");
} else if (!/^[a-z0-9=_\-\.\/]*$/.test(this.state.groupId)) {
} else if (!/^[a-z0-9=_\-./]*$/.test(this.state.groupId)) {
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem right

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Current version

\. matches the character . literally (case sensitive)
\/ matches the character / literally (case sensitive)

Proposed version:
./ matches a single character in the list ./ (case sensitive)

Same thing

src/components/views/elements/Pill.js
@@ -29,7 +29,7 @@ const REGEX_MATRIXTO = new RegExp(MATRIXTO_URL_PATTERN);

// For URLs of matrix.to links in the timeline which have been reformatted by
// HttpUtils transformTags to relative links. This excludes event URLs (with `[^\/]*`)
const REGEX_LOCAL_MATRIXTO = /^#\/(?:user|room|group)\/(([#!@+])[^\/]*)$/;
const REGEX_LOCAL_MATRIXTO = /^#\/(?:user|room|group)\/(([#!@+])[^/]*)$/;
Copy link
Member

Choose a reason for hiding this comment

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

This also doesn't seem right... I think...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to https://regex101.com/r/1M2H7B/2/

\/ matches the character / literally (case sensitive)

and

/ matches the character / literally (case sensitive)

so they are the same

src/phonenumber.js
@@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

const PHONE_NUMBER_REGEXP = /^[0-9 -\.]+$/;
const PHONE_NUMBER_REGEXP = /^[0-9 -.]+$/;
Copy link
Member

Choose a reason for hiding this comment

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

Also seems a bit weird... maybe

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These also seem to be the same https://regex101.com/r/1M2H7B/3

turt2live
turt2live approved these changes Oct 29, 2018
View reviewed changes
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

Thank you!

@turt2live turt2live merged commit c2b7df7 into matrix-org:develop Oct 29, 2018
@aaronraimist aaronraimist deleted the lint branch October 29, 2018 12:44
@dbkr
Copy link
Member

dbkr commented Jan 9, 2019

Thanks for the PR, although just a couple of notes since I just got slightly surprised by this: I'd been writing async () => ... (with a space). In future it might be better to set the option in eslint (in this case "space-before-function-paren": ["error", { "asyncArrow": "always" } ]) to match the exiting precedent when there is one like this. Also we ought to keep eslint versions similar between all 3 projects (ie. react-sdk is now enforcing no-space async but js-sdk is not).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@turt2live turt2live turt2live approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@aaronraimist @turt2live @dbkr