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

add v0.dev to data.json #2286

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

add v0.dev to data.json #2286

wants to merge 4 commits into from

Conversation

sk337
Copy link

@sk337 sk337 commented Sep 4, 2024

add v0.dev to data.json

Copy link
Member

@ppfeister ppfeister left a comment

Choose a reason for hiding this comment

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

The url just points to the homepage and has no {} for substituting in a username, nor is there any request data provided as an alternative. This does not appear to check any username, and would return 200 for all queries to the homepage.

The username_unclaimed key is also long deprecated, and this will fail schema validation.

@sk337
Copy link
Author

sk337 commented Sep 5, 2024

The url just points to the homepage and has no {} for substituting in a username, nor is there any request data provided as an alternative. This does not appear to check any username, and would return 200 for all queries to the homepage.

The username_unclaimed key is also long deprecated, and this will fail schema validation.

fixed the URL and the deprecated field

@sk337 sk337 marked this pull request as draft September 5, 2024 20:08
},
"v0.dev": {
"errorType": "message",
"errorMessage": "404 - Page Not Found",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"errorMessage": "404 - Page Not Found",
"errorMsg": "404 - Page Not Found",

Copy link
Member

Choose a reason for hiding this comment

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

Note that you can test against the schema locally via tox

@ppfeister
Copy link
Member

Are you aware of any request method that will return a different status code rather than needing an error message? While messages are acceptable, they're a bit of a last resort as they cause problems with l10n

@sk337
Copy link
Author

sk337 commented Sep 5, 2024

Are you aware of any request method that will return a different status code rather than needing an error message? While messages are acceptable, they're a bit of a last resort as they cause problems with l10n

I did some testing and noticed that invalid users return 200 will do more testing didn't check for that in the start

@ppfeister
Copy link
Member

No worries if not. Status codes are preferred but sometimes it's not that easy.

Once the suggested change to errorMsg is made, it should be compliant with the schema. Just gotta make sure the error message works once it's valid and able to be tested.

If you're testing locally, you can point sherlock to your own file with --json as well

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