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

Added ability to add warp points to other directories #121

Merged
merged 33 commits into from
May 14, 2024

Conversation

p1r473
Copy link
Contributor

@p1r473 p1r473 commented May 12, 2024

Added new function to allow you to add a bunch of directories quickly without needing to cd to them first.
addcd (path) Adds a path to your warp points with the directory's name
addcd (path) (point) Adds a path to your warp points with a custom name

Copy link
Collaborator

@alpha-tango-kilo alpha-tango-kilo left a comment

Choose a reason for hiding this comment

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

Nice job fixing the buffer issue!

Out of interest, can't you implement wd_addcd using wd_add? Is there a particular reason why you opted to keep everything separate?

Also, some tests for the new functionality would be nice :)

Final aside, it would be nice if the buffer fix was a separate PR to the new feature. That way, I could merge in the feature improvement while we work through the new feature feedback. No worries if it's too difficult to separate at this point though

@p1r473
Copy link
Contributor Author

p1r473 commented May 12, 2024

The reason I implemented addcd separate was because addcd and add can both take one parameter - add can take a point and addcd can take a path. What would be the best way to distinguish? Also, a path and a point have different failures such as a point can't have a slash and a path must exist as a directory

I definitely prefer it to all be in 1 function but I couldn't figure out a logical way to implement that because both can take one parameter and then it's tough to know if it's a path or a point

Sure I'll move the buffer fix to its own PR!

Thanks!

@p1r473 p1r473 changed the title Fixed fzf browse, added ability to add warp points to other directories Added ability to add warp points to other directories May 12, 2024
@alpha-tango-kilo
Copy link
Collaborator

In pseudocode, what I'm imagining is

wd_addcd() {
    cd $dir_param
    wd_add $(dirname $dir_param)
}

Would that work?

@p1r473
Copy link
Contributor Author

p1r473 commented May 12, 2024

Leave this with me you just sparked some ideas!

Copy link
Contributor Author

@p1r473 p1r473 left a comment

Choose a reason for hiding this comment

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

boom! please review. love your feedback!

Copy link
Collaborator

@alpha-tango-kilo alpha-tango-kilo left a comment

Choose a reason for hiding this comment

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

Loving the test coverage! Just asking some questions mostly for my understanding, but I also don't see where the logic is handling wd addcd with only one arg

wd.sh Show resolved Hide resolved
wd.sh Show resolved Hide resolved
wd.sh Show resolved Hide resolved
test/tests.sh Outdated Show resolved Hide resolved
@p1r473
Copy link
Contributor Author

p1r473 commented May 13, 2024

but I also don't see where the logic is handling wd addcd with only one arg

there were 4 tests I made, this one does that:

test_wd_addcd_path_only()
{
    create_test_wp
    wd -q addcd "$WD_TEST_DIR"
    assertTrue "should successfully add default wp for directory" \
        "$(wp_exists "$(basename "$WD_TEST_DIR")")"
    destroy_test_wp
}

I've merged all the tests into one just for wd_cd now

@p1r473 p1r473 requested a review from alpha-tango-kilo May 13, 2024 21:41
Copy link
Collaborator

@alpha-tango-kilo alpha-tango-kilo left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for documenting the tests nicely. Just that last question I re-opened and then I'll merge & release this

Copy link
Contributor Author

@p1r473 p1r473 left a comment

Choose a reason for hiding this comment

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

Answered Q

@alpha-tango-kilo alpha-tango-kilo merged commit a5f5b17 into mfaerevaag:master May 14, 2024
1 check passed
@alpha-tango-kilo
Copy link
Collaborator

Released as v0.7.0!

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.

2 participants