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

Refactoring DB Manager Plugin to use list comprehensions and generators #41153

Merged
merged 7 commits into from Jan 25, 2021

Conversation

dericke
Copy link
Contributor

@dericke dericke commented Jan 25, 2021

Description

Within the DB Manager plugin, this PR replaces for loops with equivalent list comprehensions where possible, and some list comprehensions with generators. In two cases, I also squashed some conditionals together, and in one case I squashed dict population into the intial creation. This should make the altered methods somewhat faster.

@github-actions github-actions bot added this to the 3.18.0 milestone Jan 25, 2021
@elpaso
Copy link
Contributor

elpaso commented Jan 25, 2021

unrelated test failure

continue
ids.append(self.quoteId(i))
return u'.'.join(ids)
return u'.'.join(
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return u'.'.join(
return '.'.join(

While reworking this, the u prefix is no longer needed.

@roya0045
Copy link
Contributor

If this aims to modernize things, shouldn't we use the format or even f-string instead of the old % method for strings?

@dericke
Copy link
Contributor Author

dericke commented Jan 25, 2021

@roya0045 and @m-kuhn , I limited the types of changes I made to avoid making a huge PR, but I would be happy to do a more extensive rework of these files if that would be welcomed.

@roya0045
Copy link
Contributor

My nitpick was mostly for one line. Though I'm not sure if this refactoring gives any advantage or is mostly a matter of style change. But I highly dislike the % formatting. But I don't think its worth changing them for the sake of it, at the moment, maybe except on the lines you modified.

@m-kuhn
Copy link
Member

m-kuhn commented Jan 25, 2021

@roya0045 and @m-kuhn , I limited the types of changes I made to avoid making a huge PR, but I would be happy to do a more extensive rework of these files if that would be welcomed.

Sounds good to me 👍

@m-kuhn m-kuhn merged commit f180ef9 into qgis:master Jan 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants