Added message in setup_runtime scripts #1226
Conversation
Some users reported wondering if anything (good or bad) happened when sourcing these This let them know what happen Some users also thought this only needed to be done once for all and would be persistent accross terminal/sessions/reboots, etc...
|
@dlonie @aashish24 @chaosphere2112 @remram44 please review |
|
Should probably also display feedback line 13. |
|
You mean in cleanup? Or when it fails? |
|
Exactly before line 13. In that branch, you cleanup and still return 0 without any output. |
|
Good idea, I'll add a line there. |
adds a message when UVCDAT was already sourced at the current location
|
@remram44 done. |
There was a problem hiding this comment.
why can't the var be outside the quotes?
There was a problem hiding this comment.
actually per: http://wiki.bash-hackers.org/syntax/quoting the quotes are probably not even needed here...
There was a problem hiding this comment.
echo "Nothing to do UVCDAT is already setup at ${UVCDAT_SETUP_PATH}" 1>&2
or
echo "Nothing to do UVCDAT is already setup at \"${UVCDAT_SETUP_PATH}\"" 1>&2
are probably what you meant
There was a problem hiding this comment.
i definitely don't want the quotes in the output, and since we are doing an echo of strings the quotes are actually not necessary. What I have in there is right. But I really don't care and we want to keep @remram44 happy so I will push an update right away with the quotes around the var.
There was a problem hiding this comment.
How about:
'Nothing to do since UVCDAT is already setup at '${UVCDAT_SETUP_PATH}
|
@aashish24 @remram44 I think we are good to go now. Right? |
There was a problem hiding this comment.
I made this an error because this does NOT setup the runtime environment, it keeps the previous one (which is different if this branch is reached).
There was a problem hiding this comment.
I think INFO should be good enough. Its not an error if we kept the last one. The script executed fine it just that it didn't do anything.
There was a problem hiding this comment.
I disagree: the script failed to load the requested environment. It should return an error code.
You wouldn't make the UV-CDAT setup process return 0 if it failed but another UV-CDAT is already installed.
There was a problem hiding this comment.
actually @remram44 is right, it should be an error here, we failed to use/setup the UVCDAT that the user requested. reversing.
There was a problem hiding this comment.
@remram44 what's important here? Users get the information right? whether you and I tag it as INFO or ERROR they will see the message. If you want we can change it to WARNING but its not a system error by any means in my opinion.
There was a problem hiding this comment.
You run the script for one reason, that is to load that UV-CDAT version in your shell. If you can't load it, it is as fatal an error as it can be. While would you pretend otherwise?
The only place the error status matters is when run from another script. If you run this from another script and encounter the condition, something is very wrong. Why would you possibly want to hide that?
I get that you like picking on whatever I say, but breaking the current behavior on purpose just because I wrote it is childish. UNIX has a notion of error codes, why not use it? Sure, you can figure it out by reading the terminal messages, but seriously, why not report errors as errors?
|
👍 |
|
@aashish24 @remram44 I think we're good to go now. |
|
🆗 👍 |
|
@doutriaux1 for the sake or moving forward, I am okay with this change. |
Added message in setup_runtime scripts
Some users reported wondering if anything (good or bad) happened when sourcing these
This let them know what happened
Some users also thought this only needed to be done once for all and would be
persistent accross terminal/sessions/reboots, etc...