search

MrYsLab / pymata4

A High Performance Python Client For Arduino Firmata
GNU Affero General Public License v3.0
76 stars 30 forks source link

Replace print functions #14

Closed rbarzic closed 4 years ago

rbarzic commented 4 years ago

Replace calls to print function with call to logging.{info,warning,error} to avoid polluting terminal when doing terminal-rich or gui application

MrYsLab commented 4 years ago

Thank you for the pull request. I am undecided as to whether I will be merging this or not. It is not that I don't appreciate the effort you made, but the reason is, that I offered a logging feature in an earlier version of pymata when requested, only to learn that the requester used my code in a commercial application and did not have the courtesy to provide attribution for my efforts as I had asked. I do not request or accept personal contributions, but do expect users to at least provide mention of my work in their documentation. A simple "thank you" is all I ask.

Also, it is unclear to me why logging is necessary. For a GUI, the print statements should go to the console, and for console applications, it is just a few lines that appear. It has saved me untold hours in helping users debug their applications.

If I do decide to merge the pull request, I will be adding an init parameter to select this feature, with the default being logging disabled.

I will be making a decision within the next week or so. It would be helpful if you could explain how the print statements are disruptive for your purposes.

Again thanks.

rbarzic commented 4 years ago

Hi Alan,

I understand your concern. For me, a module printing stuff directly on stdout is a big no no as it destroys the layout of my application (I'm using modules like rich, PyInquirer and halo to have an "enhanced" apparence while still having a console application. I use the logging module for all my debug and use one or several -v/--verbose options to set the logging level) - Same for my GUI apps - logging messages are going to a specific window, different from the main console Anyway - it is not a big deal for me if the patch is not applied - I've developed those applications for the internal use of the company I'm working for and I can easily manage to install the patched version of your module from github

Best regards

Ronan

On Tue, May 26, 2020 at 4:57 PM Alan Yorinks notifications@github.com wrote:

Thank you for the pull request. I am undecided as to whether I will be merging this or not. It is not that I don't appreciate the effort you made, but the reason is, that I offered a logging feature in an earlier version of pymata when requested, only to learn that the requester used my code in a commercial application and did not have the courtesy to provide attribution for my efforts as I had asked. I do not request or accept personal contributions, but do expect users to at least provide mention of my work in their documentation. A simple "thank you" is all I ask.

Also, it is unclear to me why logging is necessary. For a GUI, the print statements should go to the console, and for console applications, it is just a few lines that appear. It has saved me untold hours in helping users debug their applications.

If I do decide to merge the pull request, I will be adding an init parameter to select this feature, with the default being logging disabled.

I will be making a decision within the next week or so. It would be helpful if you could explain how the print statements are disruptive for your purposes.

Again thanks.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/MrYsLab/pymata4/pull/14#issuecomment-634078170, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAGGD4TTHMP32OTC3YZZX6LRTPKETANCNFSM4NKGBHFQ .

MrYsLab commented 4 years ago

Here is what I decided. If the project you are working on is willing to provide attribution for pymata4 in its documentation, then I am willing to do the merge. However, I will need a link to that documentation to verify it. If the attribution does not appear or is removed, then I will revert the merge. Here are the details for the feature: The feature will be disabled by default but can be enabled when instantiating pymata4. I am planning on a release between now and the end of next week. Thanks

MrYsLab commented 4 years ago

I am closing this pull request. If you would like to discuss reopening it, please leave a comment here.