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.

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

Use template literals instead of string concatenation #3066

Merged
merged 5 commits into from
Mar 19, 2023

Conversation

KristjanESPERANTO
Copy link
Contributor

We have used it inconsistently till now. Template literals are more modern and easier to maintain in my opinion.

Because that's a large amount of changes, here's a way to reproduce it: I added the rule "prefer-template": "error" to the .eslintrc.json and did an autofix. Since this caused a new problem in line 409 of newsfeed.js, I reversed it in that line and also removed the rule from the eslint config file.

The rule is described here: https://eslint.org/docs/latest/rules/prefer-template

Note: I've played around with some other linter rules as well, and some seem to point to some specific, non-cosmetic, issues. But before I dive even deeper and then introduce even bigger and hardly understandable changes at once, I thought I'd start with this simple cosmetic rule.

We have used it inconsistently so far. Template literals are more modern and usually easier to read.
https://eslint.org/docs/latest/rules/prefer-template
@khassel khassel requested a review from rejas March 14, 2023 20:30
@rejas
Copy link
Collaborator

rejas commented Mar 19, 2023

Good PR. I would vote for adding the rule to the eslint config and add one "// disable" exception to the line in the newsfeed module. That way we stay "modern" and can fix the line maybe later :-)

Copy link
Collaborator

@rejas rejas left a comment

Choose a reason for hiding this comment

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

add rule to eslintrc and add exception to the one line in newsfeed module

@KristjanESPERANTO
Copy link
Contributor Author

I just did the following:

  • fixed the problematic line
  • added the rule ("prefer-template": "error")
  • removed a redundant rule ("prettier/prettier": "error") - This change has nothing to do with the other changes, but is too minor for an extra PR.

@codecov-commenter
Copy link

Codecov Report

Merging #3066 (59d184b) into develop (8f8945d) will increase coverage by 0.10%.
The diff coverage is 22.63%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@             Coverage Diff             @@
##           develop    #3066      +/-   ##
===========================================
+ Coverage    23.02%   23.13%   +0.10%     
===========================================
  Files           52       52              
  Lines        11582    11566      -16     
===========================================
+ Hits          2667     2676       +9     
+ Misses        8915     8890      -25     
Impacted Files Coverage Δ
js/loader.js 0.00% <0.00%> (ø)
js/main.js 0.00% <0.00%> (ø)
js/module.js 0.00% <0.00%> (ø)
js/socketclient.js 0.00% <0.00%> (ø)
js/translator.js 0.00% <0.00%> (ø)
modules/default/alert/notificationFx.js 0.00% <0.00%> (ø)
modules/default/clock/clock.js 0.00% <0.00%> (ø)
modules/default/compliments/compliments.js 0.00% <0.00%> (ø)
modules/default/newsfeed/newsfeed.js 0.00% <0.00%> (ø)
modules/default/weather/providers/envcanada.js 0.00% <0.00%> (ø)
... and 19 more

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@rejas rejas merged commit d276a7d into MagicMirrorOrg:develop Mar 19, 2023
@KristjanESPERANTO KristjanESPERANTO deleted the lint branch March 19, 2023 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants