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

tidy "NO CLUE WHAT THIS DOES OR WHY IT'S NEEDED" #515

Closed
LeeReid1 opened this issue Nov 30, 2023 · 2 comments · Fixed by #516
Closed

tidy "NO CLUE WHAT THIS DOES OR WHY IT'S NEEDED" #515

LeeReid1 opened this issue Nov 30, 2023 · 2 comments · Fixed by #516

Comments

@LeeReid1
Copy link

While trying to debug something of my own, I found your apply transforms code contains the following

            # NO CLUE WHAT THIS DOES OR WHY IT'S NEEDED
            for jj in range(len(myargs)):
                if myargs[jj] is not None:
                    if myargs[jj] == '-':
                        myargs2 = [None]*(len(myargs)-1)
                        myargs2[:(jj-1)] = myargs[:(jj-1)]
                        myargs2[jj:(len(myargs)-1)] = myargs[(jj+1):(len(myargs))]
                        myargs = myargs2

It seems to be trimming hyphens from the start of each argument..?

Something like this might be simpler:

myargs = [None if s is None else s.lstrip('-') for s in myargs]

Though I note the first will remove only one hyphen and mine will remove all. Not sure what was intended.

Apologies that I have no time to test or do a pull request. Hope that helps though

@cookpa
Copy link
Member

cookpa commented Nov 30, 2023

Thanks for flagging this. I think it's wanting to remove instances of '-' from the myargs list, but in a very weird way that won't actually do anything.

If myargs contains '-', it will cause an error because it shortens myargs within a loop while iterating over the original index length.

If myargs does not contain '-', it won't do anything.

@stnava @ntustison am I missing a use case where stripping '-' from the args list might be needed? Is there a reason to fix this rather than just remove it entirely?

@ntustison
Copy link
Member

Not that I'm aware of. I have no idea why this snippet is there.

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 a pull request may close this issue.

3 participants