Skip to content

Commit

Permalink
Fix pylint issues in "genio"/"log_file" and warn about it's issues
Browse files Browse the repository at this point in the history 
The "genio"/"log_file" is quite dangerous and requires using private
members of "genio" module. Unfortunatelly "Avocado-vt" heavily depends
on this so let's just fix style issues and add docstrings explaining the
issues.

Signed-off-by: Lukáš Doktor <[email protected]>
  • Loading branch information
@ldoktor
ldoktor committed Jun 13, 2018
1 parent e6e7063 commit 2d31545
Showing 1 changed file with 17 additions and 6 deletions.
23 changes: 17 additions & 6 deletions aexpect/utils/genio.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,18 +9,29 @@
#
# See LICENSE for more details.

"""
Naive module that keeps tacks of some opened files and somehow manages them.
"""

import os

_open_log_files = {}
# This variable is used from Avocado-vt
_open_log_files = {} # pylint: disable=C0103

This comment has been minimized.

Sign in to view

Copy link
@pevogam

pevogam Jun 13, 2023

Contributor

Hi @ldoktor, what is the reason for this module with its private variable to be here instead of Avocado VT? From what I see not only does this introduce a lot of strange coupling between projects but it is a private module attribute that is then accessed as a global variable on the Avocado VT side. Btw I stumbled upon this while investigating the issue in avocado-framework/avocado-vt#3677 (comment). Do you happen to know more about the motivation behind this module overall?

This comment has been minimized.

Sign in to view

Copy link
@ldoktor

ldoktor Jun 26, 2023

Author Contributor

Hello @pevogam, I'm sorry for a late reply, there was KVM forum and DevConf, still getting through the backlog. Unfortunately I don't have a good reply for you, I just know it's being used in Avocado-vt and "always" was, therefore nobody wants to touch it. If you're brave enough, you can...

This comment has been minimized.

Sign in to view

Copy link
@pevogam

pevogam Aug 14, 2023

Contributor

@luckyh Do you think an endeavor in this direction is worth it for Avocado VT? I could propose a pull request if you confirm it will be of sufficient use, after all a proper boundary between the separate projects is really desirable.

This comment has been minimized.

Sign in to view

Copy link
@luckyh

luckyh Aug 28, 2023

Contributor

@pevogam sorry for the late reply. I think it'd be definitely great to have this unnecessary coupling issue to be fixed. However, since I haven't researched deeper into it, I'm not sure if there would be a simple approach requiring a patch to be applied just on the VT side, or we have to do a little bit complicated refactoring for both the two projects. Anyway, we could have the PR to be proposed first since you already had the idea, then we could discuss there.

Thanks!

This comment has been minimized.

Sign in to view

Copy link
@pevogam

pevogam Oct 16, 2023

Contributor

Sure, the pull requests on each side are:

  1. avocado-framework/avocado-vt#3782
  2. #124

This is just PoC meant to collect feedback if the overall approach is good at first, then we can iron out the kinks.


def close_log_file(filename):

"""
This closes all files that use the same "filename" (not just path, but
really just "basename(filename)".
"""

remove = []
for k in _open_log_files:
if os.path.basename(k) == filename:
f = _open_log_files[k]
f.close()
remove.append(k)
for log_file in _open_log_files:
if os.path.basename(log_file) == filename:
log_fd = _open_log_files[log_file]
log_fd.close()
remove.append(log_file)
if remove:
for key_to_remove in remove:
_open_log_files.pop(key_to_remove)

0 comments on commit 2d31545

Please sign in to comment.