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

refactor Pythonic sake #1077

Merged
merged 61 commits into from
Mar 17, 2020
Merged

refactor Pythonic sake #1077

merged 61 commits into from
Mar 17, 2020

Conversation

mgaitan
Copy link
Collaborator

@mgaitan mgaitan commented Feb 9, 2020 •
edited
Loading

A general refactor to simplify common patterns a make a the codebase a bit more pythonic. Flatten code, reduce complexity, less lambdas, and more

I've also augmented tests (and simplify them using pytest helpers) in few cases.

mgaitan added 30 commits February 9, 2020 02:24
@mgaitan
simplify max fps pattern
7e8f88d
@mgaitan
use all instead not any and non-empty list checking
565a339
@mgaitan
dont need to be lists to iterate
33da2ea
@mgaitan
replace hasattr by getattr
a2a700b
@mgaitan
give a generator instead a list
328b765
@mgaitan
simple conditional assignation
a338db1
@mgaitan
set booleans to add readabiity
3a0d74f
@mgaitan
unsloppy code
bfbcb82
@mgaitan
use ternary expressions and flatten if/elif
65914d0
@mgaitan
truthy object
a8b0f8c
@mgaitan
simplify find_extension usage
6deed31
@mgaitan
remove not used variable
f276997
@mgaitan
ternary expression
27dfd95
@mgaitan
more on conditions
dd58a6c
@mgaitan
reverse condition in a sake of readability
678471c
@mgaitan
etattr with default instead hasattr (flatten code)
4c365bf
@mgaitan
simple credits parsing
83420bc
@mgaitan
space after operators
878c86c
@mgaitan
use iter
8bede70
@mgaitan
pass generators instead list
1381c4e
@mgaitan
indenting
fb8c956
@mgaitan
indentation
d042b5b
@mgaitan
more indention
9e89197
@mgaitan
truthy check
f7115dc
@mgaitan
clean offset calculation
e92d86d
@mgaitan
explicit filter condition in a loop
b876f6d
@mgaitan
useless import
725ebb2
@mgaitan
use super
346875f
@mgaitan
not explicity empty list checking
f9b1c60
@mgaitan
bool condition
c22f12a
@tburrows13 tburrows13 added needs-more-info Waiting for submitter's reply, feedback, updates,... refactor Does not affect the end user at all i.e. making code easier to read or PEP8 compliant. labels Feb 24, 2020
@mgaitan
Copy link
Collaborator Author

mgaitan commented Feb 26, 2020

@tburrows13 thanks for the careful review. I'm in the middle of a trip now, but gonna update this following your comment ASAP, probably during the next wekeend.

@tburrows13 tburrows13 added this to the Release v2.0.0 milestone Feb 26, 2020
@tburrows13
Copy link
Collaborator

Sure, no hurry

@mgaitan mgaitan requested a review from tburrows13 March 5, 2020 04:35
@mgaitan
Copy link
Collaborator Author

mgaitan commented Mar 6, 2020

this is ready for a new review/merge .

@mgaitan mgaitan requested a review from Zulko March 6, 2020 13:00
@tburrows13 tburrows13 removed the needs-more-info Waiting for submitter's reply, feedback, updates,... label Mar 9, 2020
@tburrows13 tburrows13 removed the review-needed PRs which need to be reviewed (further) by project maintainers. label Mar 9, 2020
@mgaitan
Copy link
Collaborator Author

mgaitan commented Mar 12, 2020

So, could we merge this or an extra review is needed?

@mgaitan mgaitan mentioned this pull request Mar 15, 2020
Closed
@mgaitan
Copy link
Collaborator Author

mgaitan commented Mar 17, 2020

@Zulko could you check and merge this one? It's blocking the other refactors proposed.

Alternatively, give me permission on the repo. (I'll use it with care, promise)

@tburrows13 tburrows13 merged commit f22e663 into Zulko:master Mar 17, 2020
@tburrows13
Copy link
Collaborator

I've got no issues with merging, so I'll go ahead and do it. I'd happily support giving @mgaitan the Collaborator role as well. I think if you merge the current master branch into your other PRs, then they'll be sorted now.

@tburrows13 tburrows13 added the tests Related to individual tests in the test suite or running the test suite. label Mar 17, 2020
@mgaitan
Copy link
Collaborator Author

mgaitan commented Mar 17, 2020

excellent, thanks!

@tburrows13
Copy link
Collaborator

Sure! I've been away this last weekend, but now that my university term has finished, I'm keen to get v1.0.2 out within a few days. Feel free to comment on any PRs that you think would be suitable and I can add them to the milestone.

@Zulko
Copy link
Owner

Zulko commented Mar 17, 2020

Thank you so much guys for taking care of the library! I am a bit out of touch but super happy to see that!

Martin I sent you a collaborator invitation, maybe add your name in the maintainers list in the Readme? Maybe also mark Tom and yourself as main maintainers, since you are currently the most up-to-date on the library?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tburrows13 tburrows13 tburrows13 approved these changes

@Zulko Zulko Awaiting requested review from Zulko

Assignees
No one assigned
Labels
refactor Does not affect the end user at all i.e. making code easier to read or PEP8 compliant. tests Related to individual tests in the test suite or running the test suite.
Projects
None yet
Milestone
Release v1.0.2
Development

Successfully merging this pull request may close these issues.
4 participants
@mgaitan @Zulko @coveralls @tburrows13