search

NOAA-GFDL / fre-cli

Python-based command line interface for FRE (FMS Runtime Environment) to compile and run FMS-based models and post-process their output.
GNU Lesser General Public License v3.0
3 stars 11 forks source link

adding notification for which directory cloned into #42

Closed cwhitlock-NOAA closed 8 months ago

cwhitlock-NOAA commented 8 months ago

Yes, it's just the print statement. It's a hello world for understanding the merge process.

bcc2761 commented 8 months ago

@cwhitlock-NOAA Sorry I didn't get back to you on Friday; I actually worked on a fix for this as soon as I saw your issue submitted, but I still want to get your merge in. Would you mind making a new commit with this change?

https://github.com/bcc2761/fre-cli/commit/e48e227695907e464317878fbab29bc87f749f45

Click prefers click.echo over print (reference: https://click.palletsprojects.com/en/8.1.x/quickstart/#echoing), and I thought that moving this notification to the end would be better than before the cloning occurs. Let me know if you have any thoughts

cwhitlock-NOAA commented 8 months ago

Thank you for the heads-up about click.echo!

I do have a quibble about the timing of the clone notification: If you don't print out the full path of where the clone is taking place before the clone command, the error from a failed clone command only prints the last two elements of the directory path (i.e. the experiment directory and the clone dir).

I added that print, in part, because I spent an embarrassing amount of time confusing the cylc-src and cylc-run directories a few weeks back when I was trying to clean up an experiment. I don't think this fix is going to prevent users from getting foggy at 4 PM on a Friday, but printing the full path should make it easier to figure out what they are doing wrong.

On Mon, Mar 4, 2024 at 11:32 AM Bennett Chang @.***> wrote:

@cwhitlock-NOAA https://github.com/cwhitlock-NOAA Sorry I didn't get back to you on Friday; I actually worked on a fix for this as soon as I saw your issue submitted, but I still want to get your merge in. Would you mind making a new commit with this change?

@.*** https://github.com/bcc2761/fre-cli/commit/e48e227695907e464317878fbab29bc87f749f45

Click prefers click.echo over print (reference: https://click.palletsprojects.com/en/8.1.x/quickstart/#echoing), and I thought that moving this notification to the end would be better than before the cloning occurs. Let me know if you have any thoughts

— Reply to this email directly, view it on GitHub https://github.com/NOAA-GFDL/fre-cli/pull/42#issuecomment-1976994648, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC362WXEUNNKEGMYZHU3XN3YWSOZRAVCNFSM6AAAAABECMC2VWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNZWHE4TINRUHA . You are receiving this because you were mentioned.Message ID: @.***>

-- Carolyn Whitlock

bcc2761 commented 8 months ago

Good point. I didn't account for failed clones that might confuse the user. Yeah, we can add it before the clone occurs then