Add eslint rule to find unused properties, data and computed properties #5347

MisRob · 2019-04-06T16:34:10Z

Summary

Implement eslint rule that reports unused properties, data, and computed properties
Turn it on and deal with it
The last commit contains fix for the following issue: One of the reported errors showed that there was a development mock label in a group exercise lesson report link leading back to a parent lesson page (Coach -> Reports -> Groups –> select a group lesson <lesson_title> -> select an exercise belonging to this lesson and watch backlink label (should be 'Back to "<lesson_title>"' but was 'Back to “Lesson 1"')

Before

After

Reviewer guidance

Break it :)! Any ideas about possibly untreated situations or edge cases?
I tried to be careful and check that there is no hidden usage for all properties removed in
055bfc3 , though I would appreciate if someone had a look
In 582dcab I removed constants from properties. This could also be solved be leaving them in properties but referencing them using this. I guess there's no need to save them to an instance and it's also a bit more visible that they are some common definitions and constants, but I don't have any preference so if someone prefers the second solution, no problem.
In 9ca2dae I commented out all unused properties related to to commented out template TODO code. I could remove everything but I suppose that it is under development? If it's not the case, I'll remove everything including those template parts.

References

Partially solves #4992.

As discussed in #4992, as soon as it's ready, I would like to open the rule PR directly in eslint-plugin-vue repo where there's already an issue for this rule opened and proposal accepted.

Contributor Checklist

PR process:

PR has the correct target branch and milestone
PR has 'needs review' or 'work-in-progress' label
If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
If this is an important user-facing change, PR or related issue has a 'changelog' label
If this includes an internal dependency change, a link to the diff is provided

Testing:

Contributor has fully tested the PR manually
If there are any front-end changes, before/after screenshots are included
Critical user journeys are covered by Gherkin stories
Critical and brittle code paths are covered by unit tests

Reviewer Checklist

Automated test coverage is satisfactory
PR is fully functional
PR has been tested for accessibility regressions
External dependency files were updated if necessary (yarn and pip)
Documentation is updated
Contributor is in AUTHORS.rst

Those constants are referenced directly, they're actually not used as a component instance data.

that are needed for TODO template commented out code.

codecov · 2019-04-06T16:58:21Z

Codecov Report

Merging #5347 into develop will increase coverage by 0.12%.
The diff coverage is 40%.

Impacted Files	Coverage Δ
...ts/src/views/common/notifications/ActivityList.vue	`55.28% <ø> (ø)`	⬆️
...sets/src/views/plan/CoachExamsPage/ExamPreview.vue	`0% <ø> (ø)`	⬆️
kolibri/core/assets/src/views/ExamReport/index.vue	`27.58% <ø> (ø)`	⬆️
...olibri/plugins/user/assets/src/views/UserIndex.vue	`89.47% <ø> (+8.52%)`	⬆️
...ews/plan/LessonContentPreviewPage/QuestionList.vue	`0% <ø> (ø)`	⬆️
...learn/assets/src/views/AssessmentWrapper/index.vue	`19.52% <ø> (ø)`	⬆️
...ch/assets/src/views/plan/GroupEnrollPage/index.vue	`0% <ø> (ø)`	⬆️
...ement/assets/src/views/SelectContentPage/index.vue	`57.31% <ø> (ø)`	⬆️
...h/assets/src/views/common/HeaderTabs/HeaderTab.vue	`20% <ø> (+5.71%)`	⬆️
...sets/src/views/reports/ReportsLessonEditorPage.vue	`0% <ø> (ø)`	⬆️
... and 46 more

rtibbles

This is excellent. My read through of the code changes and the linting code make me think this is definitely close to mergeable, but as you suggest, I will also try to break it myself!

rtibbles · 2019-04-06T18:53:14Z

kolibri/plugins/coach/assets/src/views/reports/ReportsGroupHeader.vue

@@ -79,6 +79,7 @@
      group() {
        return this.groupMap[this.$route.params.groupId];
      },
+      /** TODO COACH


This looks unfinished?

rtibbles · 2019-04-06T18:53:42Z

kolibri/plugins/coach/assets/src/views/reports/ReportsGroupReportLessonPage.vue

@@ -93,6 +93,7 @@
    components: {},
    mixins: [commonCoach],
    computed: {
+      /** TODO COACH


packages/eslint-plugin-kolibri/lib/rules/vue-no-unused-properties.js

indirectlylit · 2019-04-06T19:22:40Z

awesome!

I tried to be careful and check that there is no hidden usage for all properties

One thing to check for is that many of the users of these components will still be passing in the props. For example, we deleted id from TaskProgress but that prop is still being passed in.

I removed constants

We had a related conversation a little while go. The general pattern is that some thing in <script> needs to be accessed in <template>, and then we've tended to do one of:

put this.thing = thing; or this.$options.thing = thing; in mounted() or somewhere similar
put thing in data()
put thing() { return thing; } in computed props

There are lots of these scattered around the code base. I don't think the inconsistency has caused us any particular issues, but might be nice to standardize on a strategy.

I commented out all unused properties related to to commented out template TODO code

I think some of these relate to some 0.12.y coach tools work @jonboiser is working on. @jonboiser would you mind checking if any of these are actually dead code that should be removed?

- id, contentId, channelId - related AssessmentWrapper and its parent (ContentPage) cleanup

MisRob · 2019-04-07T08:56:24Z

Thanks for info Devon!

One thing to check for is that many of the users of these components will still be passing in the props.

Right, I didn't realize I should also clean this, thanks. It's done in the last four commits.

We had a related conversation a little while go. The general pattern is that some thing in <script> needs to be accessed in <template>, and then we've tended to do one of...

OK. It was used only in the script part here but I guess it will be better to keep it consistent and prepared for usage in templates so I moved those constants I removed before back to an instance and referenced them using this (2934966).

indirectlylit · 2019-04-07T23:52:51Z

I guess it will be better to keep it consistent and prepared for usage in templates

all right, that's cool. The way you had it before also seemed fine though

MisRob · 2019-04-08T05:59:21Z

No problem, I don't have any strong opinion on this particular issue and if this is a general tendency ;)

indirectlylit

I'm gonna go ahead and merge this to prevent merge conflicts.

@jonboiser when you get back, could you follow up on the 'TODO' items and flag if there are any that won't be addressed by your work on coach tools?

jtwaleson · 2019-05-04T14:54:04Z

This is great!! Are there any plans to add this to eslint-plugin-vue? Could an update be pushed to NPM so we can use these rules via eslint-plugin-kolibri?

MisRob · 2019-05-04T15:03:17Z

Hello, we opened a PR in eslint-plugin-vue repository one month ago, you can find it here vuejs/eslint-plugin-vue#871 with some more info regarding updates and other, similar, rules.

Michaela Robosova added 7 commits April 6, 2019 16:35

Implement no unused properties eslint rule

16a0d57

Turn on vue-no-unused-properties

b2d6002

Remove unused properties

055bfc3

Remove constants from data

582dcab

Those constants are referenced directly, they're actually not used as a component instance data.

Fix property casing

d5bf7da

Comment out unused properties

9ca2dae

that are needed for TODO template commented out code.

Fix back link label

f525da8

MisRob added the work-in-progress Not ready for review label Apr 6, 2019

MisRob added this to the 0.12.y milestone Apr 6, 2019

MisRob added TODO: needs review Waiting for review and removed work-in-progress Not ready for review labels Apr 6, 2019

MisRob requested review from jonboiser, rtibbles and indirectlylit April 6, 2019 16:58

MisRob added the TAG: dev experience Build performance, linting, debugging... label Apr 6, 2019

rtibbles reviewed Apr 6, 2019

View reviewed changes

Michaela Robosova added 6 commits April 7, 2019 09:55

Define constants for properties groups and their labels

71a436d

Move constants back to data/computed and reference them using this

2934966

Remove obsolete props from ContentRender parents

ef93e54

- id, contentId, channelId - related AssessmentWrapper and its parent (ContentPage) cleanup

Remove obsolete props from TaskProgress parents

16ffff1

Remove obsolete props from DeleteUserModal parents

826780d

Remove obsolete props from ResetUserPasswordModal parents

ac3cb9f

indirectlylit approved these changes Apr 8, 2019

View reviewed changes

indirectlylit merged commit c3af46e into learningequality:develop Apr 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add eslint rule to find unused properties, data and computed properties #5347

Add eslint rule to find unused properties, data and computed properties #5347

MisRob commented Apr 6, 2019 •

edited

Loading

codecov bot commented Apr 6, 2019 •

edited

Loading

rtibbles left a comment

rtibbles Apr 6, 2019

rtibbles Apr 6, 2019

indirectlylit commented Apr 6, 2019 •

edited

Loading

MisRob commented Apr 7, 2019

indirectlylit commented Apr 7, 2019

MisRob commented Apr 8, 2019

indirectlylit left a comment

jtwaleson commented May 4, 2019

MisRob commented May 4, 2019

Add eslint rule to find unused properties, data and computed properties #5347

Add eslint rule to find unused properties, data and computed properties #5347

Conversation

MisRob commented Apr 6, 2019 • edited Loading

Summary

Reviewer guidance

References

Contributor Checklist

Reviewer Checklist

codecov bot commented Apr 6, 2019 • edited Loading

Codecov Report

rtibbles left a comment

Choose a reason for hiding this comment

rtibbles Apr 6, 2019

Choose a reason for hiding this comment

rtibbles Apr 6, 2019

Choose a reason for hiding this comment

indirectlylit commented Apr 6, 2019 • edited Loading

MisRob commented Apr 7, 2019

indirectlylit commented Apr 7, 2019

MisRob commented Apr 8, 2019

indirectlylit left a comment

Choose a reason for hiding this comment

jtwaleson commented May 4, 2019

MisRob commented May 4, 2019

MisRob commented Apr 6, 2019 •

edited

Loading

codecov bot commented Apr 6, 2019 •

edited

Loading

indirectlylit commented Apr 6, 2019 •

edited

Loading