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

feat: change transformModFileToJSON to return line and column information #177

Conversation

ewanharris
Copy link
Member

Description

Changes the return type of the transformModFileToJSON to a structure that allows us to return line and column information as part of the data to allow us to improve error reporting without tying us to the underlying yaml parser that we use on each platform.

Currently only has the JS implementation and will follow up with the Go implementation.

References

Resolves #173

Review Checklist

  • I have clicked on "allow edits by maintainers".
  • I have added documentation for new/changed functionality in this PR or in a PR to openfga.dev [Provide a link to any relevant PRs in the references section above]
  • The correct base branch is being used, if not main
  • I have added tests to validate that the change in functionality is working as expected

@ewanharris ewanharris requested a review from a team as a code owner March 4, 2024 17:56
@ewanharris ewanharris linked an issue Mar 4, 2024 that may be closed by this pull request
@d-jeffery
Copy link
Contributor

Source mapping is great: https://github.com/openfga/language/assets/1425457/52935de6-3c3f-46bf-b93e-882dc67205e7

Just questioning who should own validation of whether a file name is valid to avoid whack-a-mole.

@rhamzeh
Copy link
Member

rhamzeh commented Mar 5, 2024

Just questioning who should own validation of whether a file name is valid to avoid whack-a-mole.

I think the frontend (so VS Code/CLI should always be the one validating the files, as reading the files in a uniform way is outside the scope of language), e.g. reading the files on web vs native vs code

@ewanharris
Copy link
Member Author

@rhamzeh I think @d-jeffery was referring to the validation the language does currently here where it ensure that it ends with .fga, do you think we should remove that?

Myself and Daniel had discussed potentially moving the .fga check to be a "return file with errors" case and then changing the returned structure to be like { modFile, errors }, that would allow us to potentially extend in future if we ever wanted to have warnings rather than just errors.

@d-jeffery
Copy link
Contributor

@rhamzeh I think @d-jeffery was referring to the validation the language does currently here where it ensure that it ends with .fga, do you think we should remove that?

Yes, this is what I was referencing.

@d-jeffery
Copy link
Contributor

From a quick talk with @rhamzeh

agree on: file loading and validation should be done by the "UI" (VS Code ext, CLI, etc..) and file name validation being in language (this happens before loading)

also agree on language returning a json blob containing errors with a level

@ewanharris ewanharris merged commit affcf39 into feat/148-support-modular-models Mar 6, 2024
12 checks passed
@ewanharris ewanharris deleted the chore/improve-fga-mod-file-reporting branch March 6, 2024 10:58
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.

Issues in validation of fga.mod fields
3 participants