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

Introduce tags for categorization and optional argument --game #2088

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Gregory-rt
Copy link

This pull request resolves the issue #2084 . As it was discussed, categorization seems like a useful feature and this pr addresses it with the introduction of tags (Kudos to @ppfeister for the proposal). This required a few modifications, in order to handle nsfw sites, since the boolean isNSFW in the json files was replaced with the String "nsfw" in the site's tags. In addition, optional argument --game was added, which helps search through only game related sites, using the tag "game".

@ppfeister
Copy link
Member

Oh nice I didn't expect to see a pr already
I'm gonna have to check it out and see how it affects things, maybe this weekend

@sdushantha
Copy link
Member

This is a great feature to be added, but I beleive a --tags tags1,tag2 option would be best when we look at this feature in the long term. This way, we wont have to manually add a new flag for each tag.

Another idea would be to have flags for inlcuding and excluding tags. This way, user can decide to only query sites under the "game" catagory or query all sites except those under the "game" catagory.

@ppfeister
Copy link
Member

ppfeister commented May 4, 2024

I was messing with a --tags change before getting caught up elsewhere, so glad to see that get mentioned.

Exclusions would be pretty nice. Perhaps just a - before each listed item. --tags gaming,-nsfw would include items with the gaming tag while excluding those that also have the nsfw tag. --tags -gaming would query all sites except for gaming.

My one concern @sdushantha is with regression for already-pulled users. Bit of an afterthought after originally proposing tags. The switch from isNsfw to tags: nsfw would cause currently pulled clients to perform as if everything is sfw. Should we retain the isNsfw key alongside for a while just to reduce the number of would-be-outdated clients that are still in the wild? After a lil time passes, we can remove isNsfw is much less of an impact.

Adding some sort of versioning to the sites[.]json would possibly help as a forward-looking fix. Let the client check if the version number has advanced, and defer to local if so. Version bump on breaking changes only.

@sdushantha
Copy link
Member

Exclusions would be pretty nice. Perhaps just a - before each listed item. --tags gaming,-nsfw would include items with the gaming tag while excluding those that also have the nsfw tag. --tags -gaming would query all sites except for gaming.

Great idea @ppfeister, this is very intuitive in my opinion

My one concern @sdushantha is with regression for already-pulled users. Bit of an afterthought after originally proposing tags. The switch from isNsfw to tags: nsfw would cause currently pulled clients to perform as if everything is sfw. Should we retain the isNsfw key alongside for a while just to reduce the number of would-be-outdated clients that are still in the wild? After a lil time passes, we can remove isNsfw is much less of an impact.

Adding some sort of versioning to the sites[.]json would possibly help as a forward-looking fix. Let the client check if the version number has advanced, and defer to local if so. Version bump on breaking changes only.

I agree that keeping the isNsfw is wise to prevent issues for users who have the version of Sherlock which uses isNsfw. As you mentioned, it a versioning of the sites.json file should be created

@ppfeister
Copy link
Member

ppfeister commented May 6, 2024

So.... reply-to: @sdushantha

Played around with schema versioning earlier, and ultimately decided against it. It sounded like a good idea at first, but in the end, it just seemed repetitive. Not quite what was hoped.

The whole point of having __version__ in the main is for breaking changes and impactful feature changes such as this, so it ended up creating two version checks that resulted in the same remedial action. It would probably make more sense to just increment the minor, which existing clients already check for.

After bumping the minor, we can greenlight removal of isNsfw after a few weeks or a month or so

@sdushantha
Copy link
Member

@ppfeister Incrementing the minor seems like an effective solution

ppfeister added a commit to ppfeister/sherlock that referenced this pull request May 8, 2024
Added exception catch for TypeErrors due to future addition of keys, allowing Sherlock to continue past those errors.
Removed $schema to accomodate older versions of the parser. This key will be added back in sherlock-project#2088 (or other version incrementing change).
@ppfeister ppfeister mentioned this pull request May 8, 2024
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