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

[FEAT-596] Add multiple tag filter support #2105

Conversation

kyletaylored
Copy link
Contributor

@kyletaylored kyletaylored commented Dec 4, 2020

The summary of this PR is extending the functionality of the existing --tag option to support multiple tags as a comma-separated string, in addition to adding a --tags option which is more explicit. These also support single tag values as to not alter the previous behavior.

  • --tag will return sites matching ANY tag
  • --tags will return sites that match ALL tags
  • Both options will return all sites with NO tags if the value is an empty string (--tag "")

I added 3 methods to the TerminusCollection in addition to the binary has() method:

  • containsAny - if any provided ID matches
  • containsAll - if all provided IDs match
  • containsNone - Match if an empty ID was provided with sites with no ID

What about --filter?
While the --filter option can work, it is more restrictive in terms of both functionality and results. The tag field is parsed as a single, combined string, which causes unexpected results.

  1. Single tag (--filter "tags=tagA")
    This will only return a list of sites that have only this one tag. If the site has other tags, this fails.

  2. More than one tag (--filter "tags=tagA||tags=tagB")
    Similar to a single tag, only shows a list of sites if they solely contain each tag, and it disregards other tags.

  3. More than one tag, but listed alphabetically (--filter "tags=tagA,tagB")
    Will only list sites that have both tags (none others), and they must be listed in alphabetical order. If you were switch the order of the tags (tagB,tagA), then you will get no results.

- Add the ability to provide a comma-separated list of tags that will return a list of sites that have ANY tag in the list.
- Add an additional --tags options which takes a comma-separated list of tags, but requires ALL tags to match.
- Now supports NO tags and will return only sites with no tags if the tag option value is empty.
Comment on lines +65 to +67
if (!is_null($tags = $options['tags'])) {
$this->sites->filterByTags($tags);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

tag and tags probably shouldn't be used together, unless it's used in an and/or combination? Maybe there is a use for it.

@kyletaylored kyletaylored changed the title Add multiple tag filter support [FEAT-596] Add multiple tag filter support Dec 10, 2020
- Add the ability to provide a comma-separated list of tags that will return a list of sites that have ANY tag in the list.
- Add an additional --tags options which takes a comma-separated list of tags, but requires ALL tags to match.
- Now supports NO tags and will return only sites with no tags if the tag option value is empty.
…d/terminus into feature/add-multitag-support
@stovak stovak changed the base branch from master to v3.0 June 28, 2021 23:23
@stovak stovak changed the base branch from v3.0 to master June 28, 2021 23:25
@stovak
Copy link
Member

stovak commented Jun 28, 2021

Sorry, that we're just now getting to this, but we haven't really had anyone working on Terminus in a while. I'll try to rebase around the changes we're including in v3.0 and and see if I can tease out just your tag changes. Gimme a day or so to work out the diff.

@greg-1-anderson
Copy link
Member

Note also that the underlying API that Terminus uses does not report all tags, so this PR will not behave correctly in all circumstances.

@kyletaylored
Copy link
Contributor Author

@greg-1-anderson What is the underlying API that is used, and do you have recommendations on how to address those? Or just pointing out the potential issue as a heads up?

@kyletaylored
Copy link
Contributor Author

@greg-1-anderson @stovak Would it be possible to revisit this PR? Getting the right tag data from the platform API should be a separate issue, but in this case we're just applying the correct logic to the data that is available. // cc @danny2p

@kporras07
Copy link
Contributor

Terminus 2 is EOL now so closing this PR. We'll get this merged into Terminus 3 soon

@kporras07 kporras07 closed this Feb 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Invalid behaviour in Sites::filterByTag() when sites have no tags Support for Multiple Values for TAG?
4 participants