Skip to content

Conversation

@gaelicWizard
Copy link
Contributor

@gaelicWizard gaelicWizard commented Sep 18, 2021

Description

New function to do a search looking for a sibling to a parent of the current directory, for example to find ../../.git to indicate that $PWD is inside a git repository.

Motivation and Context

This removes the remaining path traversal logic in the gradle plugin and completion, and I suspect may be useful for SCM stuff in themes.

Suggestions for a better function name welcome!!

How Has This Been Tested?

All tests pass.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code follows the code style of this project.
  • If my change requires a change to the documentation, I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • If I have added a new file, I also added it to clean_files.txt and formatted it using lint_clean_files.sh.
  • I have added tests to cover my changes, and all the new and existing tests pass.

@gaelicWizard gaelicWizard marked this pull request as ready for review September 18, 2021 21:56
@gaelicWizard
Copy link
Contributor Author

@cornfeedhobo I don't mean to spam you, but you said you'd have time! ♥

@gaelicWizard gaelicWizard force-pushed the uncle branch 2 times, most recently from f942c1f to 5f56674 Compare September 19, 2021 10:07
@cornfeedhobo
Copy link
Member

@gaelicWizard Hey no worries. I did indeed invite it.

This looks good and I like what you're thinking. Here are my thoughts and questions:

  • I usually think of these functions as "walking" the tree, but I tried to come up with a good name and couldn't come up with anything. At the very least, I would like to make sure any new functions added are prefixed with _bash-it- to make intent, scope, and ownership clear.
  • It looks like gw() isn't setting variables outside the function scope, so maybe result could be made a local?
  • I notice that _find_uncle is using () instead of {} which implicitly opens a subshell. Since we don't use this approach very often, and because we want to keep the barrier to entry low for contribution, could you please add some comments on your reasoning for doing that.
  • I noticed that you enforce the check that PWD is set with ${PWD?}, but don't set a message. What's your thinking/reasoning here?

Thanks again for all your hard work! This is great stuff.

@gaelicWizard
Copy link
Contributor Author

  • I noticed that you enforce the check that PWD is set with ${PWD?}, but don't set a message. What's your thinking/reasoning here?

I just linted the themes folder and put ? on ten thousand things and didn't even have the thought that maybe using a shell built-in parameter doesn't need it 😆

@gaelicWizard
Copy link
Contributor Author

  • I tried to come up with a good name and couldn't come up with anything

According to the British Journal of General Practice (via NIH's National Library of Medicine) There isn't a word for it and a bunch of answers on Stack Overflow...there isn't a word for it...

but I saw someone say "kin" so now I'mma try that! "uncle" just seems too...old male person

@gaelicWizard
Copy link
Contributor Author

  • It looks like gw() isn't setting variables outside the function scope, so maybe result could be made a local?

It is! 😜 The local result is at the top of the function. SC2155 discourages putting them together. 👍

@gaelicWizard gaelicWizard force-pushed the uncle branch 2 times, most recently from 65f6fca to 4ab0089 Compare September 19, 2021 16:25
@cornfeedhobo
Copy link
Member

  • I tried to come up with a good name and couldn't come up with anything

According to the British Journal of General Practice (via NIH's National Library of Medicine) There isn't a word for it and a bunch of answers on Stack Overflow...there isn't a word for it...

but I saw someone say "kin" so now I'mma try that! "uncle" just seems too...old male person

@gaelicWizard How about _bash-it-find_in_ancestors()?

@gaelicWizard gaelicWizard changed the title lib/helpers: new function _find_uncle() lib/helpers: new function _bash_it_find_kin() Sep 19, 2021
@NoahGorny
Copy link
Member

I guess find_in_ancestors is a pretty good name

@gaelicWizard gaelicWizard changed the title lib/helpers: new function _bash_it_find_kin() lib/helpers: new function _bash_it_find_in_ancestor() Sep 20, 2021
@gaelicWizard
Copy link
Contributor Author

Ok! I rewrote the function to take any number of files to look for, renamed it to _bash_it_find_in_ancestor(), and rebased on current master.

@gaelicWizard gaelicWizard force-pushed the uncle branch 2 times, most recently from f5a2324 to b456863 Compare September 20, 2021 21:25
Copy link
Contributor

@davidpfarrell davidpfarrell left a comment

Choose a reason for hiding this comment

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

Kind of funny I just coded up a similar script yesterday:

Script called findup that walks up the path but uses standard find elements for your search query.

I have some comments on the function - Overall likely a useful addition to the library !

@gaelicWizard
Copy link
Contributor Author

ok, I've just pushed an updated version

@gaelicWizard gaelicWizard changed the title lib/helpers: new function _bash_it_find_in_ancestor() lib/helpers: new function _bash-it-find-in-ancestor() Sep 22, 2021
Copy link
Contributor

@davidpfarrell davidpfarrell left a comment

Choose a reason for hiding this comment

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

I think we're getting closer !

New function to do a search looking for a sibling to a parent of the current directory, for example to find `../../.git` to indicate that `$PWD` is inside a git repository.
Add `composure.sh` citation with examples and rewrite internal comments to describe the code flow.
@gaelicWizard
Copy link
Contributor Author

gaelicWizard commented Sep 23, 2021

@davidpfarrell, @cornfeedhobo, @NoahGorny, it seems like 'returns' isn't actually one of our standard citations. Would it be helpful if I started adding these to other functions as I go?

ALSÖ, what about 'about' versus '_about'?

Copy link
Contributor

@davidpfarrell davidpfarrell left a comment

Choose a reason for hiding this comment

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

Looks like we might still have some citation stuffs to work out (about, return, etc).
I don't have a lot to say on that so I'll leave it to others.

I'm happy with the parts I was concerned about, so I'm approving it, although there might still be some work before it gets merged ...

@davidpfarrell
Copy link
Contributor

/off-topic

ALSÖ

OK so at this point the Ö feels like a purposeful style choice, so I have to ask :

  • How did you develop this habit ?
  • Does it take extra time to type or do you have a system-level auto-correct configured for it?

#inquiring_minds_want_to_knöw

@gaelicWizard
Copy link
Contributor Author

gaelicWizard commented Sep 23, 2021

#inquiring_minds_want_to_knöw

At some point in the ancient past, I added an autocarrot for "alsö" as seen in Mønti Pythøn ik den Høli Gräilen. 😆 🤣

Copy link
Member

@NoahGorny NoahGorny left a comment

Choose a reason for hiding this comment

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

well done @gaelicWizard and great review @davidpfarrell 😃

@NoahGorny NoahGorny merged commit 7fb7bb9 into Bash-it:master Sep 28, 2021
@gaelicWizard gaelicWizard deleted the uncle branch September 29, 2021 22:59
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.

4 participants