-
Notifications
You must be signed in to change notification settings - Fork 16
feat: add markdown to JSON parser enhancements (label normalization + schema validation) #195
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
base: develop
Are you sure you want to change the base?
Conversation
|
Hey praneel, thanks alot for working on this feature.
Actually, I was thinking if we should change the workflow of adding maintainers. I'll experiment with this thoroughly (idk when) |
|
Sounds good, let me know if you have/face any issues with this |
agriyakhetarpal
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Praneel7015, thanks for asking for my review on this PR. I am not a maintainer for this project, and I don't work for FOSS United either (I'm happy to help nevertheless), so I'll leave it to Dilip to guide you through the rest of this PR and to review it in more detail beyond my initial skim through the code. Thanks for your work! Some comments below:
| if schema is None: | ||
| print("Schema file not found, skipping validation", file=sys.stderr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the schema file is available next to this file, will there be a case where it won't be found? I think this check would be redundant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that i think about it, you're right. i dont think there will be a case where that occurs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, I think this file is redundant, as it reimplements functionality already available for use via JSON schema checkers, as I mentioned in the original issue description: #18 (comment). Not to say this is bad, but it's probably better to rely on existing, tested tools, which are less prone to bugs. Maybe it's easier to let people fix any errors instead of auto-fixing entries?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes Sense, my thoughts were having it automated will be easier for all 😅, but you are also right its basically just redundant file which does the same thing as what the JSON schema checker would do.
will look into removing it!
Co-authored-by: Agriya Khetarpal <74401230+agriyakhetarpal@users.noreply.github.com>
Co-authored-by: Agriya Khetarpal <74401230+agriyakhetarpal@users.noreply.github.com>
Co-authored-by: Agriya Khetarpal <74401230+agriyakhetarpal@users.noreply.github.com>
Co-authored-by: Agriya Khetarpal <74401230+agriyakhetarpal@users.noreply.github.com>
Addresses Issue #18.
checks if jsonschema is present on system
loading and validation
PS : this is still a draft to check if all conditions are being met and if the issue is resolved with this
This is just the start, expect more to come :)