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.

Sign up for GitHub

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

Jump to bottom

Various improvements in code #825

Merged
merged 11 commits into from
Jan 19, 2024
Merged

Various improvements in code #825

merged 11 commits into from
Jan 19, 2024

Conversation

rdbende
Copy link
Collaborator

@rdbende rdbende commented Dec 14, 2023

Fixes for some things I noticed:

  • simplify if statements
  • use early-returns where possible
  • update type annotations to more modern syntax in some places
  • delete unused code

naglis
naglis reviewed Dec 15, 2023
View reviewed changes
Copy link
Contributor

@naglis naglis left a comment

Choose a reason for hiding this comment

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

+72 -138 🏅

cozy/model/settings.py Show resolved Hide resolved
cozy/model/settings.py Outdated Show resolved Hide resolved
cozy/view_model/library_view_model.py Outdated Show resolved Hide resolved
rdbende and others added 2 commits December 15, 2023 13:15
@rdbende @naglis
Apply changes suggested by @naglis
8abcb62 
Co-authored-by: Naglis Jonaitis <827324+naglis@users.noreply.github.com>
@rdbende
Improve string formatting for logs
c1aec75
rdbende
rdbende commented Dec 18, 2023
View reviewed changes
cozy/media/gst_player.py Outdated Show resolved Hide resolved
@rdbende rdbende requested a review from naglis January 4, 2024 21:27
@rdbende
Copy link
Collaborator Author

rdbende commented Jan 4, 2024

@naglis Could you re-review, and approve the PR if it looks good to you? I can merge PRs, but that requires an approving review first, and so far you had great comments, not only on this one.

naglis
naglis approved these changes Jan 5, 2024
View reviewed changes
Copy link
Contributor

@naglis naglis left a comment

Choose a reason for hiding this comment

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

Just a few nitpicks.

I was reluctant to express "Approval" with my comments originally as I am not very familiar with the codebase.

cozy/control/mpris.py Outdated Show resolved Hide resolved
cozy/media/gst_player.py Outdated Show resolved Hide resolved
cozy/model/settings.py Outdated Show resolved Hide resolved
cozy/view_model/library_view_model.py Outdated Show resolved Hide resolved
rdbende and others added 3 commits January 6, 2024 17:28
rdbende
rdbende commented Jan 6, 2024
View reviewed changes
cozy/model/track.py Show resolved Hide resolved
cozy/report/report_to_loki.py Show resolved Hide resolved
@rdbende rdbende requested a review from naglis January 7, 2024 10:45
geigi
geigi approved these changes Jan 19, 2024
View reviewed changes
Copy link
Owner

@geigi geigi left a comment

Choose a reason for hiding this comment

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

Very nice, looks good to me, thanks!

@geigi geigi merged commit 4388c6d into geigi:master Jan 19, 2024
3 of 4 checks passed
@rdbende rdbende mentioned this pull request Jan 19, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@naglis naglis naglis approved these changes

@geigi geigi geigi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rdbende @naglis @geigi