Revise require imports #3071
Revise require imports #3071
Conversation
- order (external first) - remove superfluous file extensions - new line after imports - deconstruct (only one time (in `check_config.js`)) - fix path (only one time (in `global-setup.js`))
Codecov Report
📣 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 #3071 +/- ##
===========================================
+ Coverage 27.51% 27.56% +0.05%
===========================================
Files 52 52
Lines 11566 11569 +3
===========================================
+ Hits 3182 3189 +7
+ Misses 8384 8380 -4
... 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. |
|
I am wondering if something like https://github.com/import-js/eslint-plugin-import/blob/main/docs/rules/order.md would be applicable to our setup so that future requires are already sorted? |
|
what's the point of these cosmetic changes, and all the files impacted for no functional reason. |
|
@rejas Yes, I have worked with this plugin and the rule. But I'm not ready to implement them yet. There are still some problems with the Since I don't know yet when I could integrate the plugin, I wanted to bring in these improvements already. Or would it make more sense to set the PR to draft for so long? @sdetweil Things like unified code and modern coding conventions increase code quality and make projects easier to maintain. Functional changes are easier to integrate. Sometimes you also discover bugs that you wouldn't have seen otherwise. Also, I personally learn better how the code works and can help more when problems occur. If you want to read deeper, the topics Code review and Code refactoring are good starting points. |
|
I understand all the principles, and the objectives, just not the process. yes, when u change a file for a functional reason, then apply these cosmetic changes. don't change the files JUST for cosmetic purposes. if it's not broke, you don't apply mass changes for policy purposes. been doing this a long time. you don't change coding practices to match your developers. never once has the alphabetical order of imports been functionally useful, and some times the order matters |
Some changes are cosmetic but the purpose is not cosmetic. The purpose is cleaner code which is easier to maintain and has fewer bugs.
The sense of refactoring is exactly the opposite of that statement. Refactoring is a systematic process of improving code without creating new functionality that can transform a mess into clean code and simple design. (https://refactoring.guru/refactoring) Note: I don't want to say that MM is a mess code 😄
This PR is not just about alphabetical order. With this PR, it may not be entirely clear where the whole process is going, I'll admit that. Are there no review or refactoring processes in your professional projects? In my understanding, this is part of best practice. |
|
review and refactoring are engaged when the error rate or new function demands it. not just because it's a good long term policy. you'll do whatever. I disagree. fought this battle over and over and it never turns out for the best. |
|
Apparently we have had very contrasting experiences.
How do you mean that? I submitted a PR and you are free to reject it. No hard feelings 🙂 |
|
You both argue well, and I can see both sides of the argument. Still, I favor merging this PR. So, whats the third opinion here @khassel ? |
|
already approved this |
|
I am about to implement the linter rule for this. Please wait a few more minutes. |
|
Okay, the tests are running successfully locally and here at GitHub. I'm done with this PR 🙂 In the future I will try to include the linter rule right away. |
|
the change here in |
|
other point: Do we really have no test for serveronly? |
I have an idea to solve this without making an exception to the new rule here. I will make a PR this tomorrow. How did you find the issue? |
|
I have a serveronly setup running as website and I built a new docker image with this changes which is substituted automatically via watchtower so my mm was down (container restarting). |
To fix issue caused by MagicMirrorOrg#3071
Ah I see.
I can't see a test for serveronly, but it would be important. I can't create one at the moment. Do we open an issue to address later? Or do you have the time? |
To fix issue caused by #3071 In order to still fulfill the new linter rule `import/order`, I replaced the alias with the path. I also see two other approaches, but I opted for the simplest one here for now. The other approaches: 1. Also create an alias for `app`. 2. Use new `imports` (like in PR #2934), but let `_moduleAliases` untouched to stay compatible to 3rd party modules. **Edit**: Oh, I thought of another option: - Add `require("module-alias/register");`
|
Created an issue so whoever has time and energy can do it :-) |
## [2.23.0] - 2023-04-04 Thanks to: @angeldeejay, @buxxi, @CarJem, @dariom, @DaveChild, @dWoolridge, @grenagit, @Hirschberger, @KristjanESPERANTO, @MagMar94, @naveensrinivasan, @nfogal, @psieg, @rajniszp, @retroflex, @SkySails and @tomzt. Special thanks to @khassel, @rejas and @sdetweil for taking over most (if not all) of the work on this release as project collaborators. This version would not be there without their effort. Thank you guys! You are awesome! ### Added - Added increments for hourly forecasts in weather module (#2996) - Added tests for hourly weather forecast - Added possibility to ignore MagicMirror repo in updatenotification module - Added Pirate Weather as new weather provider (#3005) - Added possibility to use your own templates in Alert module - Added error message if `<modulename>.js` file is missing in module folder to get a hint in the logs (#2403) - Added possibility to use environment variables in `config.js` (#1756) - Added option `pastDaysCount` to default calendar module to control of how many days past events should be displayed - Added thai language to alert module - Added option `sendNotifications` in clock module (#3056) ### Removed - Removed darksky weather provider - Removed unneeded (and unwanted) '.' after the year in calendar repeatingCountTitle (#2896) ### Updated - Use develop as target branch for dependabot - Update issue template, contributing doc and sample config - The weather modules clearly separates precipitation amount and probability (risk of rain/snow) - This requires all providers that only supports probability to change the config from `showPrecipitationAmount` to `showPrecipitationProbability`. - Update tests for weather and calendar module - Changed updatenotification module for MagicMirror repo only: Send only notifications for `master` if there is a tag on a newer commit - Update dates in Calendar widgets every minute - Cleanup jest coverage for patches - Update `stylelint` dependencies, switch to `stylelint-config-standard` and handle `stylelint` issues, update `main.css` matching new rules - Update Eslint config, add new rule and handle issue - Convert lots of callbacks to async/await - Revise require imports (#3071 and #3072) ### Fixed - Fix wrong day labels in envcanada forecast (#2987) - Fix for missing default class name prefix for customEvents in calendar - Fix electron flashing white screen on startup (#1919) - Fix weathergov provider hourly forecast (#3008) - Fix message display with HTML code into alert module (#2828) - Fix typo in french translation - Yr wind direction is no longer inverted - Fix async node_helper stopping electron start (#2487) - The wind direction arrow now points in the direction the wind is flowing, not into the wind (#3019) - Fix precipitation css styles and rounding value - Fix wrong vertical alignment of calendar title column when wrapEvents is true (#3053) - Fix empty news feed stopping the reload forever - Fix e2e tests (failed after async changes) by running calendar and newsfeed tests last - Lint: Use template literals instead of string concatenation - Fix default alert module to render HTML for title and message - Fix Open-Meteo wind speed units
check_config.js))global-setup.js))