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

WebUI: Correctly apply changed "save path" of RSS rules #21030

Merged
merged 1 commit into from
Jul 8, 2024

Conversation

glassez
Copy link
Member

@glassez glassez commented Jul 6, 2024

Closes #20141.

@glassez glassez added the WebUI WebUI-related issues/changes label Jul 6, 2024
@glassez glassez added this to the 4.6.6 milestone Jul 6, 2024
@glassez glassez requested review from a team July 6, 2024 08:36
@@ -742,12 +745,14 @@
$("lastMatchText").textContent = "QBT_TR(Last Match: Unknown)QBT_TR[CONTEXT=AutomatedRssDownloader]";
}

if (rulesList[ruleName].torrentParams.stopped === null)
// use `== null` to check for both `null` and `undefined`
if (rulesList[ruleName].torrentParams.stopped == null)
Copy link
Member Author

Choose a reason for hiding this comment

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

As per https://stackoverflow.com/questions/2559318/how-to-check-for-an-undefined-or-null-variable-in-javascript:

In general it is recommended to use === instead of ==. The proposed solution is an exception to this rule. The JSHint syntax checker even provides the eqnull option for this reason.

From the jQuery style guide:

Strict equality checks (===) should be used in favor of ==. The only exception is when checking for undefined and null by way of null.

So I turned on the smart mode of ESLint eqeqeq rule so that it allows for such a comparison.

Copy link
Member

@Chocobo1 Chocobo1 Jul 6, 2024

Choose a reason for hiding this comment

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

Still I would like to enforce strict comparison operators unconditionally, it keeps the code uniform. It is verbose in these code but we only have a few instances for it (2 so far).

Copy link
Member

Choose a reason for hiding this comment

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

Also I consider verbose in this case is a plus. It become clear that it is expecting null and undefined values.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also I consider verbose in this case is a plus. It become clear that it is expecting null and undefined values.

Ok. I had a feeling about it.
I just feel difficulty understanding JS. Is the following correct?

if ((rulesList[ruleName].torrentParams.stopped === undefined) || (rulesList[ruleName].torrentParams.stopped === null))

I noticed that someone describes check for undefined as the following:

typeof(some_variable) === "undefined"

Copy link
Member

Choose a reason for hiding this comment

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

I agree that there are benefits to being verbose, but have also found this smart rule to be an improvement to convenience on other projects with much stricter codebases.

Copy link
Member Author

Choose a reason for hiding this comment

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

Anyway, it still requires verbosity either in the code or in the comment (so that someone does not break it later by replacing == with ===).

Copy link
Member

Choose a reason for hiding this comment

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

You avoid this by enforcing the general rule that all checks against null or undefined use ==.

Copy link
Member

Choose a reason for hiding this comment

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

Also I consider verbose in this case is a plus. It become clear that it is expecting null and undefined values.

Ok. I had a feeling about it. I just feel difficulty understanding JS. Is the following correct?

if ((rulesList[ruleName].torrentParams.stopped === undefined) || (rulesList[ruleName].torrentParams.stopped === null))

I noticed that someone describes check for undefined as the following:

typeof(some_variable) === "undefined"

It depends. If some_variable is declared but hasn't assigned a value then some_variable === undefined will hold. If some_variable wasn't declared then some_variable === undefined will throw exception and typeof(some_variable) === "undefined" must be used.

@glassez glassez merged commit eba5cbb into qbittorrent:master Jul 8, 2024
14 checks passed
@glassez glassez deleted the rss-rules-web branch July 8, 2024 07:08
glassez added a commit to glassez/qBittorrent that referenced this pull request Jul 8, 2024
glassez added a commit to sledgehammer999/qBittorrent that referenced this pull request Jul 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WebUI WebUI-related issues/changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Docker 4.6.2] RSS Rule save location does not save, even if other changes to the rule do save
3 participants