Skip to content

feat(exec) Filter by tags in exec cmd#344

Open
majki741 wants to merge 3 commits intoknqyf263:mainfrom
majki741:feature/filter-by-tags-in-exec
Open

feat(exec) Filter by tags in exec cmd#344
majki741 wants to merge 3 commits intoknqyf263:mainfrom
majki741:feature/filter-by-tags-in-exec

Conversation

@majki741
Copy link

Issue #, if available:

Description of changes:

Reuse of the FilterByTags method and adding the ability to filter by tags, not just a single tag, when using the 'exec' command.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@RamiAwar
Copy link
Collaborator

Looks good! Can you also add tests for this? I'm trying not to merge anything new without tests, and sometimes it's hard cause no foundation exists. Let me know if I can help you out with this if it's too much!

@majki741
Copy link
Author

majki741 commented Mar 1, 2025

the test for this function is already here: https://github.com/knqyf263/pet/blame/main/snippet/snippet_test.go#L307

@RamiAwar
Copy link
Collaborator

RamiAwar commented Mar 6, 2025

Looked into this a bit more, and I want to e2e test the filtering functionality not just this one function, but it's taking some time.

I think it's a slightly big refactor so not going to lay it on you 😄

My goal from adding these additional tests is just making sure the old functionality doesn't change with this change, but in an automated way. Sorry to keep you waiting! Will merge this soon.

@RamiAwar
Copy link
Collaborator

Sorry for the wait - this took longer than expected and I got busy. Haven't forgotten about it! Just want to add a proper test foundation for all commands. This one uses quite some things behind the scenes (ex. filtering using fish) so testing is hard, still figuring that out.

One thing that's blocking me from merging this right away is the fact that the flags changed, that's not backwards compatible. Not a big deal probably but if someone is using pet in a script we'll break it for them.

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

Comments