Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal: remove shifts into variables in favour of positional parameter to local variables #188

Open
ReDTerraN opened this issue Apr 11, 2023 · 2 comments

Comments

@ReDTerraN
Copy link
Contributor

Most of the current xpanes functions looks like this example the moment:

  local window_name="$1"
  shift
  local index_offset="$1"
  shift  
  local interval="$1"
  shift
  local repstr="$1"
  shift
  local cmd="$1"

While this works fine and from an resource optimization perspective, as multiple shifts is lightweight operations.
The readability takes a hit as all functions becomes very much larger than they need to be.

For most functions we should in theory be able to do following:

 local window_name="$1"
 local index_offset="$2"
 local interval="$3"
 local repstr="$4"
 local cmd="$5"

and in cases we need to fully reset the positional parameters we can do this instead:

  local window_name="$1"
  local index_offset="$2"
  local interval="$3"
  local repstr="$4"
  local cmd="$5"
  shift 5

@greymd is there any reason to why we utilize multiple shifting? Do you want me to proceed creating a patch to move the functions to the proposed syntax?

@greymd
Copy link
Owner

greymd commented Apr 11, 2023

@greymd is there any reason to why we utilize multiple shifting? Do you want me to proceed creating a patch to move the functions to the proposed syntax?

There isn't a particular reason, it was just my preference. Using shift was more convenient for me, as it allowed me to easily delete or swap specific elements without needing to reassign the numbers from $1 to $N. During the development phase of creating xpanes from scratch, I frequently added or deleted arguments and changed their positions.

Do you want me to proceed creating a patch to move the functions to the proposed syntax?

Thank you! Please go ahead :) As xpanes is now mature and in the phase of refinement, any changes that improve readability are welcome.

@ReDTerraN
Copy link
Contributor Author

There isn't a particular reason, it was just my preference. Using shift was more convenient for me, as it allowed me to easily delete or swap specific elements without needing to reassign the numbers from $1 to $N. During the development phase of creating xpanes from scratch, I frequently added or deleted arguments and changed their positions.

I understand it fully, Then if we have no special case to worry about and we can start working on this. As its just clean up its not top priority to fix it.

Thank you! Please go ahead :) As xpanes is now mature and in the phase of refinement, any changes that improve readability are welcome.

great! I hope when I get time I will look into this over the upcoming month's :)

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

No branches or pull requests

2 participants