search

avocado-framework / aexpect

Python library used to control interactive programs
GNU General Public License v2.0
8 stars 32 forks source link

Simplify spawn's logfile closing from using globals to other projects #124

Closed pevogam closed 11 months ago

pevogam commented 1 year ago

Arbitrary functions can be used as closing hooks for the spawns and we only need to provide a minimal logfile closing hook and functionality letting the caller design their own hooks of any further complexity.

ldoktor commented 1 year ago

Hello @pevogam, there were few ping-poings, how do you intend to approach this PR in order to reach conclusion? In general I do like it, the only issue for me is the addition of the open_fd. I haven't seen any usage of it therefore I don't think it should be included. If someone opens an issue, we can revisit it but unless there is a need of it, I'd like if you could skip it.

pevogam commented 12 months ago

Hi @ldoktor right now I am waiting for the closure of the pull request on the VT side and then can revisit all these topics. As @luckyh also added his opinion about the way to go forward here, I think it will be best to at least add a separate commit to drop the self.logfile functionality entirely and offer a single self.log_func as I wrote in https://github.com/avocado-framework/aexpect/pull/124#discussion_r1372013775. This would then wrap up everybody's concern and we will have improved aexpect API in addition to improved avocado-vt usage.

ldoktor commented 12 months ago

OK, I was afraid I was blocking you somehow. Note I have nothing against the unified log_func approach.

pevogam commented 12 months ago

OK, I was afraid I was blocking you somehow. Note I have nothing against the unified log_func approach.

No that's fine, I just hope it is also ok to sometimes contribute a change that is not complete in the views of maintainers but still a step forward passing all available tests and fully usable within the library since this is all the resource I could spare at the time.

Further improvements are always welcome on both sides but best taken a separate steps that can be easily tracked with GH issues. I would likely quantize changes in a similar manner like you if I was the aexpect maintainer (it is normal for maintainers to want the most and the best for their projects) but as a contributor I am often limited to the smallest set of changes and effort to achieve something (but of course not increase the technical debt whenever possible). As the current set of changes is already self-contained and results in net decrease of complexity (less coupling with other projects like this) and thus maintenance costs, I would prefer to take the shorter step here.

Nevertheless, as we are already more or less clear on the step forward to unify output functionality let's just go that route here once the VT pull request gets merged.

pevogam commented 11 months ago

Not sure if the CI is functioning right now so perhaps I will rebase this once we merged the CI branches and rerun.

ldoktor commented 11 months ago

The CI works well now. While on rebase would you please also remove the unused open_fd code or reference here where it's being needed?

pevogam commented 11 months ago

The CI works well now. While on rebase would you please also remove the unused open_fd code or reference here where it's being needed?

Do you still see such code? Because I have already pushed today with the final changes and only wrote about why I believe the CI fails.

pevogam commented 11 months ago

The CI passes now and this is the final version of the proposed branch, let me know if you have any further comments.

ldoktor commented 11 months ago

The CI passes now and this is the final version of the proposed branch, let me know if you have any further comments.

Ahh, you are right. It gets introduced and removed immediately. Let me merge this and in case some projects needs this we can always bring it back in a cleaner form.