Opened 4 years ago
Closed 4 years ago
#49107 closed defect (bug) (fixed)
Coding Standards: src/js/media/views/settings.js
Reported by: | ankitmaru | Owned by: | SergeyBiryukov |
---|---|---|---|
Milestone: | 5.4 | Priority: | normal |
Severity: | normal | Version: | 5.3.2 |
Component: | Media | Keywords: | has-patch dev-feedback |
Focuses: | javascript, coding-standards | Cc: |
Description
Coding Standard: Expected a conditional expression and instead saw an assignment.
Attachments (2)
Change History (8)
#1
@
4 years ago
- Component changed from General to Media
- Milestone changed from Awaiting Review to 5.4
- Owner set to SergeyBiryukov
- Status changed from new to reviewing
#3
follow-up:
↓ 5
@
4 years ago
- Resolution fixed deleted
- Status changed from closed to reopened
@ankitmaru Thanks for raising this issue, this is indeed a bit of confusing code. I'm curious what linting rule you caught this with? This code actually does an assignment and logic check, so your patch doesn't quite fix the issue.
@SergeyBiryukov: Despite the linting violation and poorly constructed code here, this change actually brakes the functionality of this code as I am reading it. The comment line above:
If the setting has a corresponding user setting, update that as well.
indicates what the code should do
the line userSetting = $setting.data('userSetting')
assigns the $setting.data('userSetting')
value and returns a truthy value if userSetting
is set, the if()
logic is checking that. After [r487029] userSetting
will be undefined at L114: window.setUserSetting( userSetting, value );
.
Instead, to clear this up and fix the linting error, we can move the assignment above the logic check:
userSetting = $setting.data('userSetting'); if ( userSetting ) { window.setUserSetting( userSetting, value ); }
#5
in reply to:
↑ 3
@
4 years ago
Replying to adamsilverstein:
Instead, to clear this up and fix the linting error, we can move the assignment above the logic check:
userSetting = $setting.data('userSetting'); if ( userSetting ) { window.setUserSetting( userSetting, value ); }
That's exactly what [47029] does, unless I'm missing something?
In 47029: