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

Add file loader and tests for compute_tab_table and load_cwd_guake_yml #2076

Merged
merged 2 commits into from
Apr 29, 2022

Conversation

jorgeecardona
Copy link
Collaborator

@jorgeecardona jorgeecardona commented Apr 20, 2022

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.

@jorgeecardona jorgeecardona changed the title Add file loader and tests Add file loader and tests for compute_tab_table and load_cwd_guake_yml Apr 20, 2022
Copy link
Collaborator

@Davidy22 Davidy22 left a comment

Choose a reason for hiding this comment

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

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:
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

A None, or a single encapsulating exception, both fine

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@Davidy22 Davidy22 left a comment

Choose a reason for hiding this comment

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

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

@Davidy22 Davidy22 merged commit 0db8148 into Guake:master Apr 29, 2022
@Davidy22
Copy link
Collaborator

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.

@jorgeecardona
Copy link
Collaborator Author

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 __init__.

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