Skip to content

Conversation

@kingshuk00
Copy link

optionally file-system for workspaces with same name in different file-systems

…-system for workspaces with same name in different file-systems
Copy link
Contributor

@cniethammer cniethammer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not 100% sure how high the value is of providing this as users can just do a simple cd $(ws_find NAME)

Comment on lines +19 to +20
WS_OPT+=" -F "
WS_OPT+=`echo ${2}`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not simply

Suggested change
WS_OPT+=" -F "
WS_OPT+=`echo ${2}`
WS_OPT+=" -F $2"

#!/bin/bash

if [ "$#" -eq 0 ]; then
echo "Usage: ws_go workspace [file-system]"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

usage info should go into separate function usage()


if [ "$#" -eq 0 ]; then
echo "Usage: ws_go workspace [file-system]"
return
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use exit instead of return

Suggested change
return
exit 1

@@ -0,0 +1,28 @@
#!/bin/bash

if [ "$#" -eq 0 ]; then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Command line parsing should be improved - looks brittle to me. Also, should (optionally) allow -F as all other ws_* tools

fi

WS_OPT=
if [ "$#" -lt 2 ]; then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Command line parsing should be improved - looks brittle to me. Second place.


WS_OPT=
if [ "$#" -lt 2 ]; then
NUMWS=`ws_find $1 | wc -l`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assign command line arguments to variables - what is $1 here in this scope?

return
fi

WS_OPT=
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you wanne WS_OPT to be later?

Suggested change
WS_OPT=
WS_OPT=""

or

Suggested change
WS_OPT=
WS_OPT=()

Comment on lines +12 to +13
NUMFS=`ws_list | grep "^ filesystem name : " | cut -d ":" -f2 | cut -d" " -f2 | sort -h | uniq | wc -l`
FSNAMES=`ws_list | grep "^ filesystem name : " | cut -d ":" -f2 | cut -d" " -f2 | sort -h | uniq | tr '\n' ' '`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

calling ws_list multiple times - use cached output.
The parsing also looks complex, involving many pipes and different commands.

Comment on lines +23 to +25
if ws_list ${WS_OPT} | grep "^id: " | cut -d":" -f2 | cut -d" " -f2 | grep -q $1; then
cd `ws_find ${WS_OPT} $1`
else
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why so over complicated. Rel on ws_find and do not call ws_list again...

Suggested change
if ws_list ${WS_OPT} | grep "^id: " | cut -d":" -f2 | cut -d" " -f2 | grep -q $1; then
cd `ws_find ${WS_OPT} $1`
else
ws_dir=$(ws_find ${WS_OPT} $1)
if [[ -d $ws_dir ]]; then
cd "$ws_dir"
else

Comment on lines +27 to +28
ws_list ${WS_OPT} | grep "^id: " | cut -d":" -f2
fi
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was obviously an error

Suggested change
ws_list ${WS_OPT} | grep "^id: " | cut -d":" -f2
fi
ws_list ${WS_OPT} | grep "^id: " | cut -d":" -f2
exit 1
fi

@holgerBerger
Copy link
Owner

actually ws_cd might be a better name?
@cniethammer and may be adding bash completion would be cool?

@cniethammer
Copy link
Contributor

@holgerBerger bash completion with the currently chosen command line argument ordering will not work - the filesystem has to be an option before the name of a workspace

@holgerBerger
Copy link
Owner

I think the main problem is that should be a shell function and not a shell script, as it can not change parent shells CWD.

@cniethammer
Copy link
Contributor

I think the main problem is that should be a shell function and not a shell script, as it can not change parent shells CWD.

I use something rather simple for this in my environment (not taking care of different file systems):

cdws ()
{ 
    local ws_name="$1";
    local ws_dir="$(ws_find "$ws_name")";
    cd $ws_dir
}

Personal opinion: As a user has to include this into his environment (.bashrc, etc.) anyway, I do not see the added value of providing this as part of the installation. A user who can set up his environment should be able to write this small function, as well at this point ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants