Skip to content
This repository was archived by the owner on Apr 26, 2023. It is now read-only.

Conversation

@delarosatrevin
Copy link
Member

Dear @rmarabini , as my review of PR #1998 ...I'm proposing some re-factoring to your ProgressBar class. I didn't want to directly commit my changes into your branch, so I have created this other PR to discuss my proposal.

Regarding the refactoring...I had two main concerns:

  • There was a lot of boiler plate code for flush, print, increment, etc...which relates somehow to the second point here
  • There was no clear API, so the access was done directly to the attributes both for getting or setting then. I have introduced a minimal set of functions...if you need access to other properties, you can add getX/setX methods...although my personal recommendation is to keep it simple and minimal.

Regarding the mystery of stdout redirection...the issue was in your code. There is a STRONG recommendation in Python to not use mutable objects (e.g { }, [ ], etc) as default values of function...because this can introduce very weird issues if you modify the default value. This was the case here...you passed as default sys.stdout, but this was evaluated before the Protocol redirection of sys.stdout took place. The solution is easy once you have found the problem...use None as default and then inside the constructor, use sys.stdout if no other output was passed as argument.

@pconesa pconesa requested a review from rmarabini July 9, 2019 08:29
@rmarabini
Copy link
Contributor

I aprove this pull request and I will check it in my own pull request (

@rmarabini
Copy link
Contributor

PLease,

do not confirm this merge, I will do it myself after I double checked it

@rmarabini rmarabini merged commit df4d863 into rm_devel Jul 15, 2019
@delarosatrevin delarosatrevin deleted the jmrt_progressbar branch July 16, 2019 12:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants