search

bit-team / backintime

Back In Time - An easy-to-use backup tool for GNU/Linux using rsync in the back
https://backintime.readthedocs.io
GNU General Public License v2.0
2.09k stars 204 forks source link

Enable lint rule W0237 (needs heavy refactoring) #1794

Closed feman323 closed 3 months ago

feman323 commented 4 months ago

Merge PR #1801 first.

buhtz commented 4 months ago

Hi Manual, thanks for this contribution. I carefully reviewed your code changes. They are not wrong. But I would take this as an opportunity to refactor the related code sections.

  1. About guiapplicationinstance.py: There is also a method check() in the base class. Looking into the code base when and how check() is called I am assuming that the modification would cause side effects. I will refactor that class and its base class but I am not sure yet how.
  2. The class OrderedSet is IMHO not needed in today's Python and used in only two methods (Snapshots.rsyncInclude() and Snapshots.rsyncExclude()). I have to take a closer look but think I can remove it. There seem to be unit tests for it. I have to check their "value". This will become a bigger refactoring. My problem with OrderedSet is also that it overwrites/shadows methods from its base class. So I am insecure and need to take a closer look.
buhtz commented 4 months ago

It will take some time (weeks maybe) until I work further on this. I see two separate refactoring tasks that need some skills and experienced developers... if someone wants to jump in. But I will take it and leave this PR open to track that tasks instead of creating new issue tickets for this.

feman323 commented 4 months ago

Hi Christian,

I'm not sure if the change in will cause any side effects. I have searched through the codebase and the only occurence a of check call with "raiseCmd" parameter was in GuiApplicationInstance, so I assume it is called only internally.

buhtz commented 4 months ago

With $ git grep ".check(" **/*.py I do see several calls. I have to check this first.

buhtz commented 4 months ago
  1. The class OrderedSet is IMHO not needed in today's Python and used in only two methods (Snapshots.rsyncInclude() and Snapshots.rsyncExclude()). I have to take a closer look but think I can remove it. There seem to be unit tests for it. I have to check their "value". This will become a bigger refactoring. My problem with OrderedSet is also that it overwrites/shadows methods from its base class. So I am insecure and need to take a closer look.

I implemented a solution for this. Will publish it after the upcoming release. #1801

buhtz commented 4 months ago

OK, about OrdereSet I opened PR #1801 . Merge #1801 first and then resolve merge conflicts.

I also reviewed point 1 and approving this solution.

Thank you for this contribution.