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

Remove singleton in config.Config #94

Closed
wants to merge 3 commits into from
Closed

Remove singleton in config.Config #94

wants to merge 3 commits into from

Conversation

lucc
Copy link
Owner

@lucc lucc commented Sep 2, 2016

This makes the Config class easier to understand. Also see commit message of c9a6330.

The singleton interface is removed to make the class and usage less confusing.
The singleton instance is replaced by a global variable in khard/khard.py.
That should make it clear that modifications to this object are persistent (in
contrast to modification to an object that was apearentrly generated on the
fly and not saved: `Config().set_editor(string)`).  It also makes the point
where the object is actually initialized much clearer.
No functional changes and no reformatting were done.
@lucc
Copy link
Owner Author

lucc commented Sep 13, 2016

@scheibler if you only have little time to spare for khard atm I would ask you to have a look at this PR first. It is very small in terms of functional changes (all of them are in c9a6330, the others are just formatting).

The second reason why I ask you to prefer this PR is that it makes rebasing and mergeing with other changes very hard (because of c3388e5) so I would like to see it merged before I have to rebase and merge it after some other PRs.

@scheibler
Copy link
Collaborator

Okay. If you are sure, that we don't need the config in other parts of the code, I will include this pr.

But should there be another release 0.11.3 before, which pins the vobject version to 0.9.2 like
suggested in #87? Although my regexp is not perfect, it works for most contacts whereas the current
situation with vobject 0.9.3 is as bad as without my workaround. I don't see another solution at
this time but to downgrade the library until they fixed the issue.

@lucc
Copy link
Owner Author

lucc commented Sep 18, 2016

Currently we do not use the config instance in any other module of khard. Any future changes to this pattern are also decisions to change it. So I would answer "we don't". Also because I think it is a nice design pattern to divide the functionality of a program as clear as possible:

  • library/backend is ignorant of the rest, all options are passed as function arguments,
  • the config code parses the config file and does not know what the values will be used for (doesn't know anything about the rest)
  • similar for the command line interface
  • main calls the cli parser, and the config parser and somehow handles the merging of these (for us main sets options in the config instance but that is fine as the logic is in main and not in config), then it calls the library and passes the correct function arguments

Such a design pattern makes it easier in my opinion to understand what happens where. It also makes it possible to use parts of the code from another program (in the future it might be possible to use the khard library part from another python script or even write a tui/gui for it (for me the library part is khard/{adress_book,carddav_object,helpers,object_type}.py).

I do not have an opinion about releasing 0.11.3 or freezing vobject. But I have a question about development: are you going to use the develop branch for development from now on (did you switch to a git-flow like pattern)? Should I make all further PRs for develop?

@scheibler
Copy link
Collaborator

I agree on your design pattern explanation.

Should I make all further PRs for develop?

Yes.

@lucc
Copy link
Owner Author

lucc commented Sep 19, 2016

I also noticed that we really need to remove the singleton stuff in order to be able to write proper tests as it will mess up the separation of test cases. Try this test:

import unittest
from khard import config


class ConfigStuff(unittest.TestCase):

    def test_config_singleton(self):
        c = config.Config()
        self.assertEqual(c.get_preferred_vcard_version(), "4.0")#or 3.0 depending on your config
        c.set_preferred_vcard_version('11')
        self.assertEqual(c.get_preferred_vcard_version(), "11")

    def test_config_singleton2(self):
        c = config.Config()
        self.assertEqual(c.get_preferred_vcard_version(), "4.0")# or 3.0 depending on your config
        c.set_preferred_vcard_version('22')
        self.assertEqual(c.get_preferred_vcard_version(), "22")


if __name__ == "__main__":
    unittest.main()

The test also fails if I put the two functions in separate classes and also if I put the classes in separate files. Save the file(s) as test_something.py and run python -m unittest.

@lucc lucc changed the base branch from master to develop September 20, 2016 21:59
@lucc
Copy link
Owner Author

lucc commented Sep 20, 2016

I chenged the merge target to develop but github doesn't understand that you merged this already. Should we close it?

@scheibler
Copy link
Collaborator

Okay, I thought, that you've been notified about merging automatically. Yes, can be closed.

@lucc lucc closed this Sep 20, 2016
@lucc lucc deleted the feature/config-class branch September 20, 2016 22:54
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

2 participants