search

fraschetti / Octoslack

OctoPrint plugin for Slack, Mattermost, Pushbullet, Pushover, Rocket.Chat, Discord, Riot/Matrix, & Microsoft Teams
MIT License
74 stars 34 forks source link

Option to post images directly to Slack #29

Closed sillyfrog closed 6 years ago

sillyfrog commented 6 years ago

This has an initial commit to make the code PEP8, then the commit of the actual code changes.

fraschetti commented 6 years ago

Hi @sillyfrog,

I finally found a bit of time to look over your changes in hopes of merging them in.

  1. Likely because this commit contains the formatting change, it's now in conflict with other changes that were merged in (minor yet still in conflict). Can you pull these into your branch and resolve accordingly?

  2. The default Progress event UpdateMethod should be NEW_MESSAGE as opposed to INLINE so as to maintain existing functionality for users who are upgrading. This should be an easy change.

  3. I partially understand why you'd not want to upload the new image to Slack in upload_snapshot but the flow for returning an open file and handing that off to the patched Slacker upload method is a bit odd. Two points here:

  4. Unless there's a justifiable reason to pass in the file directly to Slacker, wouldn't the implementation be cleaner if you gave it a file path and let the existing library read the data as per usual? I didn't spend a ton of time on it but it wasn't immediately clear to me why you'd go out of your way to override that method.

  5. In your new flow of returning a file from upload_snapshot, the finally block in upload_snapshot is going to immediately try to delete the underlying file (os.remove(local_file_path)) - regardless of whether you're returning the opened file object or the file_path (to hand off to Slacker), you likely want to null out local_file_path in upload_segment (for the Slacker case) and then ensure you handle the os.remove later in the code to ensure the local files are being cleaned up.

Once we're good to go here, I'll pull in the changes and start testing them before pushing an official release.

Thanks again for all the hard work.

sillyfrog commented 6 years ago

Thanks for getting back to me.

1: If I put in a PR now, are you able to merge right away? I'll then work on getting my other changes back in again. If it's going to take you a while, let me know and I'll do it when you are around (as you probably know, git is a bit of a horror in merging stuff back and forth like this).

2: Can do, note this is a new option, it was not available as part of the progress events previously.

Re 3-5, From memory, I think I did this to make it more "Pythonic", rather than opening and closing files all the time, pass around the file handle (that's what Slacker actually wants, but it hides it from the user in the default implementation). It'll also need a bunch of error handling to remove the file after it's used, rather than at the time it was created (but shouldn't be too hard, just means it's touching more places in the code).

Maybe if you are around now, I can put in a PR for the Black (code formatting) changes, then when that's in, I'll re-implement the above patch. (I'm not sure what editor you are using, but many support Black and similar now, or you can pip install Black, and run it on the file at any time). This will also make the diffs easier to read moving forwards.

Cheers.

fraschetti commented 6 years ago

Hi @sillyfrog I accepted your merge from today. Take your time on the changes but I will do my best to merge then asap once they're in.

Thanks again!

sillyfrog commented 6 years ago

Thanks for getting PR #31 in so fast. I have just put in PR #32 that addresses the above comments on top of this (plus sorts out a couple of other issues I cam across in my testing). If you are happy with that, then I think this PR (#29) can just be closed and not merged. Cheers!

fraschetti commented 6 years ago

Closing this particular request as these changes have been submitted via other separate requests (one for formatting and another for the logic changes)