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 'key in dict' instead of 'key in dict.keys()' #4017

Merged
merged 4 commits into from
Mar 24, 2021

Conversation

MartinThoma
Copy link
Contributor

No description provided.

Copy link
Contributor

@ferdnyc ferdnyc left a comment

Choose a reason for hiding this comment

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

👍 Drives me nuts.

I had some suggestions, if we're going to be fixing things anyway...

(Unfortunately, because they cross deleted lines in the diff, I couldn't leave them as actual suggestions that could be applied directly from the review interface.)

@ferdnyc
Copy link
Contributor

ferdnyc commented Mar 23, 2021

And here (which extended too far outside the diff to review-comment on at all)...

if name and name != os.path.basename(f.data["path"]):
f.data["name"] = name
else:
f.data["name"] = os.path.basename(f.data["path"])
if "tags" in f.data:
if tags != f.data["tags"]:
f.data["tags"] = tags
elif tags:
f.data["tags"] = tags

that can all be just:

f.data.update({"name": name or os.path.basename(f.data.get("path"))})
if "tags" in f.data or tags:
    f.data.update({"tags": tags})

No real point in those inequality tests, when any inequality results in an assignment. The lookup to get the current value may be slightly cheaper than the assignment, but it's also completely redundant whenever we have to do the assignment. Might as well skip the test and just always assign. If it happens to be the same value it already held, what's the harm?

@ferdnyc ferdnyc added the code Source code cleanup, streamlining, or style tweaks label Mar 23, 2021
Copy link
Contributor

@ferdnyc ferdnyc left a comment

Choose a reason for hiding this comment

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

LGTM! One minor typo to fix, and then I'm good to merge. Thanks!

@ferdnyc ferdnyc merged commit fc6380a into OpenShot:develop Mar 24, 2021
@MartinThoma MartinThoma deleted the sim-14 branch March 24, 2021 10:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code Source code cleanup, streamlining, or style tweaks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants