Add file loader and tests for compute_tab_table and load_cwd_guake_yml - #2076
Conversation
jorgeecardona
changed the title
Davidy22
left a comment
Davidy22
left a comment
There was a problem hiding this comment.
I notice the last three commits in this PR are fixing the tests from the commit before them, and there's some duplicate names too. Before we merge, the last four commits can probably be squashed down so we just have one commit adding the tests in main. Besides that, the other commits seem like fine subdivisions.
| content = None | ||
| try: | ||
| content = self.fm.read_yaml(filename) | ||
| except PermissionError: |
There was a problem hiding this comment.
These look like exceptions that can always occur with read_yaml(). Maybe these should be inside the function, so that future potential calls don't need to also wrap it in this chunk of exception handling?
There was a problem hiding this comment.
Sure, there are pleny of possible errors when reading files and expecting the content to have certain format. I just can't think of a good value to return when there is an error with the format, and it seems to me that returning an empty dict would obscure the fact that the file is incorrectly formatted. I could raise a ValueError I guess.
Unless you have a suggestion for a default value to return I would leave it as it is.
There was a problem hiding this comment.
A None, or a single encapsulating exception, both fine
There was a problem hiding this comment.
The method read_yaml is now returning None in the case of any "expected" error, like PermissionError , NotFoundFile, Encoding or YAMLError. If anything else happens the exception would be capture by the code in guake_app.py
1975ac3 to
503e113
Compare
08eacfe to
27e9289
Compare
27e9289 to
1203517
Compare
…ache capabilities
1203517 to
1445720
Compare
Davidy22
left a comment
Davidy22
left a comment
There was a problem hiding this comment.
Looks good, we can get away with no release notes on this one because it's pretty much part of the earlier PR. There was a strange issue a bit ago that was making CI fail that seems to be a crash bug that didn't appear until after I started merging unrelated PRs, but I went and dealt with that in 90b9bfa
|
Actually, I've been thinking about being a bit more liberal with review privileges than the project's been so far. Current repo setting is PRs need an approving review and I'm ok with that, but reviews happen quite infrequently when it's not me and I kind of want to get more people in here. Since you have a few PRs now if you want, I can give you review privileges, and it'll be up to you how often you check in and look at something on the PR queue. |
|
Great, I noticed your change in #90b9bfa and moved the initialization of the FileManager at the same point. I didn't notice any probem before, but I guess the current tests and how I manually tested didn't capture some cases where a title is loaded before that in |
2 participants
This is a continuation of #1759. Test for compute_tab_title and load_cwd_guake_yml are added and a class that controls the cache is added to utils.py (FileManager) tests for that class are also included. There are no new functionalities here, just more tests and a cleaner cache control. Also dataclasses are not used in the code that's why I removed ir from the Pipfile.