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

refactor!: Demonstrator: New config management class (and some more stuff) #1850

Draft
wants to merge 58 commits into
base: dev
Choose a base branch
from

Conversation

buhtz
Copy link
Member

@buhtz buhtz commented Aug 24, 2024

Blocked by

That PR is intended to get merged but also it is more a demonstrator instead of a modification to the current BIT behavior. The code technically is not "connected" to regular BIT and merging it won't change BIT behavior.

The objective was to replace the configuration management logic currently realized in the Config class. The current implementation is inefficient and blown up. Additionally that class does a lot more then just managing the configuration.

The PR demonstrate the new solution. Some details are not finished and need to be solved. But I would like to discuss the approach as early as possible before finishing it.

  1. See the __main__ in common/konfig.py file for a short example.
  2. I choose Konfig as name for the new class because of lack of a better alternative. I am open for suggestions. On a long run we will "delete" the whole Config class with separating its behavior in several other classes. Once when Config is gone we can rename Konfig into Config.
  3. There is only one instance of that class (singleton).
  4. Pythons in build configparser now is used to read and write the config file. Because that package reads INI-like files I used a little trick because BIT's config file is not an INI file. After reading the raw file into memory I simply add [BIT] before the first line and then read it with configparser.
  5. Profile specific configuration parameters now separated into their own class Profile internally dealing with Konfig.
  6. The creation of the backintime-config (1) man page still is done with a script. The script does not parse the konfig.py file anymore but use inspect to extract the relevant information (from public properties with docstrings).

How to integrate it into BIT? Not in this PR but later. I would use Config in a wrapper-like manner using Konfig internally. This is a lot of extra work. But this step would make it smooth and less-risky transforming the codebase into using Konfig. And also the needed PRs can be smaller. This way we can carefully decouple the codebase step by step from the Config class monolith.

Side note to myself

ConfigParser can't handle lines like this:

`qt.diff.params=%1 %2`

Problem is solved.

@buhtz buhtz added Discussion decision or consensus needed PR: Waiting for review PR won't be merged until review and approval from a member of the maintenance team. labels Aug 24, 2024
@buhtz buhtz added this to the Upcoming release milestone Aug 24, 2024
@buhtz buhtz self-assigned this Aug 24, 2024
Copy link
Contributor

@daviewales daviewales left a comment

Choose a reason for hiding this comment

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

Hi @buhtz, I've had a look over this.
I don't have any big suggestions, but I commented on a few small things.
There are a few style tweaks (particularly the suggestion for create-manpage at the end), so let me know if you'd prefer not to get these kind of style comments.

common/konfig.py Show resolved Hide resolved
common/konfig.py Outdated Show resolved Hide resolved
common/konfig.py Outdated Show resolved Hide resolved
common/konfig.py Outdated Show resolved Hide resolved
common/konfig.py Show resolved Hide resolved
common/konfig.py Outdated Show resolved Hide resolved
common/konfig.py Outdated Show resolved Hide resolved
create-manpage-backintime-config.py Outdated Show resolved Hide resolved
buhtz added a commit that referenced this pull request Sep 21, 2024
Extract the method from config.py into tools.py and refactoring it. Improved unit tests.
Related to #1850 (replacing Config class).

Close #1791
@buhtz
Copy link
Member Author

buhtz commented Sep 27, 2024

OK, I know implemented all config-fields from Config class in Konfig class as properties including man page content.

@buhtz buhtz changed the title Demonstrator: New config management class (and some more stuff) refactor!: Demonstrator: New config management class (and some more stuff) Nov 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion decision or consensus needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants