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

Fix parser regression #2109

Merged

Conversation

ppfeister
Copy link
Member

@ppfeister ppfeister commented May 8, 2024

Added catch for TypeErrors that may be caused due to the future addition of keys (if any ever get added), allowing Sherlock to continue past those errors.

Removed $schema to accomodate older versions of the parser. This key will be added back in after #2088 (or other version incrementing change) is merged, giving users time to update.


@matheusfelipeog or @sdushantha :

Merge is recommended as a hotfix.

PR addresses comments made here. Apparently the old way SitesInformation processed the data.json wasn't too friendly to the new key, so we're removing it for a now.


Fixes #2108

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).
Copy link
Collaborator

@matheusfelipeog matheusfelipeog left a comment

Choose a reason for hiding this comment

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

Okay, for me, this seems sufficient for now. @ppfeister Thank you for this.

We need to improve our tests later so that this can be caught beforehand in the PRs before merging, thus avoiding breaking anything in older versions. I'll make improvements to that in the future.

@matheusfelipeog matheusfelipeog merged commit c7f9350 into sherlock-project:master May 8, 2024
2 checks passed
@matheusfelipeog
Copy link
Collaborator

Oops, looks like I missed something here:

https://github.com/sherlock-project/sherlock/actions/runs/8995885403/job/24711519662

@ppfeister
Copy link
Member Author

Should just be an ignore-able warning from the try catch. Let me check.

@ppfeister
Copy link
Member Author

Ahhh there we go. I got it. Stand by.

@ppfeister
Copy link
Member Author

opened #2110
seems to work on my end.

@ppfeister
Copy link
Member Author

We need to improve our tests later so that this can be caught beforehand in the PRs before merging, thus avoiding breaking anything in older versions. I'll make improvements to that in the future.

Interesting. Can't imagine that being too difficult to set up.
Checkout both the new and old, run old pointing to new data.json. If it blows up, warn.

@matheusfelipeog
Copy link
Collaborator

Checkout both the new and old, run old pointing to new data.json. If it blows up, warn.

image

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.

SHERLOCK// ERROR "string indices must be integers, not 'str' "
2 participants