search

linuxmint / sticky

A sticky notes app for the linux desktop
GNU General Public License v2.0
187 stars 40 forks source link

Refactoring of the code #79

Closed DomHeadroom closed 2 years ago

DomHeadroom commented 2 years ago

I have rewrote the code in a more pythonic way, change string interpolation with f string, removed unused library/variable but there more unused variable that i have not remove!

mtwebster commented 2 years ago

Please have a look at my comments.

In light of those issues, I'm concerned over a potential lack of testing here. This is a pretty broad swath of changes - they're positive ones, for sure, but they can create new regressions, and care must be taken to perform enough testing to check out all possible code paths to changes here.

Also, please squash things into one or two logical commits. As I mentioned in one of your other PRs, something like one for imports, anothere for syntax isn't a bad idea.

collinss commented 2 years ago

Separate pull requests would also be a viable option. TBH, part of the reason I haven't gotten to this yet is because there's a lot to review in this one pull request, but if each unrelated change is in it's own pr, it's a lot easier to manage, and potential changes can be easier to address as well (just glancing over a couple of your changes I can see things I'd like changed further before merging).

mtwebster commented 2 years ago

I don't know that it needs separate PRs, just a smaller group of commits - one for each 'type' of changes would be fine, but it's up to you.