Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings
This repository was archived by the owner on Apr 26, 2023. It is now read-only.

Refactoring proposal for ProgressBar class, fixed stdout mystery #2005

Merged
rmarabini merged 2 commits into rm_devel from jmrt_progressbar
Jul 15, 2019

Conversation

@delarosatrevin
Copy link
Member

@delarosatrevin delarosatrevin commented Jul 5, 2019

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
Copy link
Contributor

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

Copy link
Contributor

PLease,

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

delarosatrevin reacted with thumbs up emoji

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

Reviewers

1 more reviewer

@rmarabini rmarabini rmarabini approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /