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

Validate DAG schema on Monaco #659

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from
Draft

Conversation

vhespanha
Copy link

This PR implements DAG schema validation in the Monaco Editor, like suggested here: issue #652

Changes

  1. Schema and Definition Updates

    • Modified the definition struct and JSON schema to accept both array and string representations of the Params field.
    • Implemented type conversion logic to handle both formats.
  2. Monaco Editor Integration

    • Integrated schema validation within the Monaco Editor.
  3. Error Handling

    • By default, the diagnostics from the validation are displayed on browser alerts (from my understanding this is how the repo usually handles Monaco diagnostics).
    • The error messages in alerts are currently raw diagnostic outputs and not user-friendly.

Next steps

The current implementation is functional but i'm considering it a draft since the approach of modifying the JSON schema to handle dual Params representation probably needs review, I'm willing to work on better diagnostics if all is right with the current approach.

Copy link

codecov bot commented Aug 10, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 12 lines in your changes missing coverage. Please review.

Project coverage is 66.02%. Comparing base (a10fff5) to head (36b986b).

Files Patch % Lines
internal/dag/loader.go 36.84% 11 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #659      +/-   ##
==========================================
- Coverage   66.16%   66.02%   -0.15%     
==========================================
  Files          53       53              
  Lines        4156     4174      +18     
==========================================
+ Hits         2750     2756       +6     
- Misses       1186     1197      +11     
- Partials      220      221       +1     
Files Coverage Δ
internal/dag/builder.go 78.59% <100.00%> (+0.12%) ⬆️
internal/dag/loader.go 66.66% <36.84%> (-5.75%) ⬇️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a10fff5...36b986b. Read the comment docs.

@yohamta
Copy link
Collaborator

yohamta commented Aug 12, 2024

Hi @vhespanha, thank you so much for your fantastic work and excellent suggestions! Really appreciate it, I see that this is a hard issue to handle.

Regarding Schema and Definition Updates:
I think adding array format support for parameters is a great idea. Let's definitely move forward with that. Maybe we can add support map as well in the future.

Regarding Monaco Editor Integration:
This looks really promising! I'm excited to see how this improves the experience. Thank you very much!

Regarding Error Handling:
For now, I believe your current approach is suitable.

I did try to test the changes on my end, but it seems the schema validation isn't triggering in my environment. This could potentially be an issue on my side. I'm curious - how is it functioning in your development environment? Could you share some details or screenshots about how you're seeing it work?

Once again, thank you for your hard work!

@vhespanha
Copy link
Author

Hey!
Here's how it's working on my machine:
image
I get this browser alert whenever the schema is not valid, I'll investigate it further and see if I can reproduce it from a fresh git pull.

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