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 loading a schema from registry #489

Merged
merged 16 commits into from
Jan 8, 2024
Merged

Conversation

jl-santana
Copy link
Contributor

Link to the issue here

Changes on this PR

  • Refactored Editor file,
    -- remove side effects from reformatAll function, solving the main problem of the issue
    -- simplified useEffects with logic related to loading the schema, separating concern and focusing them on only one task
    -- remove dead code
    -- simplified other functions

  • Refactored logic to validate the schema
    -- only validate schemas different from the default

  • Fixed issue when generating types

Screen.Recording.2024-01-05.at.10.26.09.AM.mov

@jl-santana jl-santana marked this pull request as ready for review January 5, 2024 15:42
@jl-santana jl-santana requested a review from a team as a code owner January 5, 2024 15:42
frontend/src/components/Editor/Editor.jsx Show resolved Hide resolved
frontend/src/components/Editor/Editor.jsx Show resolved Hide resolved
frontend/src/components/Editor/Editor.jsx Show resolved Hide resolved
frontend/src/components/Editor/Editor.jsx Show resolved Hide resolved
frontend/src/utils/pgSchemaTypeGen.js Show resolved Hide resolved
frontend/src/utils/validators.js Outdated Show resolved Hide resolved
@darunrs darunrs requested a review from roshaans January 5, 2024 18:08
Copy link
Contributor

@roshaans roshaans left a comment

Choose a reason for hiding this comment

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

Overall good refactoring and simplification of logic!

Just some side effects that these changes had in the behavior of the editor which we should resolve.

frontend/src/components/Editor/Editor.jsx Outdated Show resolved Hide resolved
frontend/src/components/Editor/Editor.jsx Show resolved Hide resolved
frontend/src/utils/pgSchemaTypeGen.js Show resolved Hide resolved
frontend/src/utils/validators.js Outdated Show resolved Hide resolved
frontend/src/utils/validators.js Outdated Show resolved Hide resolved
frontend/src/components/Editor/Editor.jsx Outdated Show resolved Hide resolved
frontend/src/components/Editor/Editor.jsx Outdated Show resolved Hide resolved
@jl-santana jl-santana requested review from roshaans and darunrs January 5, 2024 21:03
Copy link
Contributor

@roshaans roshaans left a comment

Choose a reason for hiding this comment

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

One more minor change, but I am happy to approve right after! Overall great changes 💯

frontend/src/components/Editor/Editor.jsx Show resolved Hide resolved
@jl-santana jl-santana requested a review from roshaans January 6, 2024 03:27
Copy link
Contributor

@roshaans roshaans left a comment

Choose a reason for hiding this comment

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

LGTM! 💯

@jl-santana jl-santana merged commit b486ced into main Jan 8, 2024
2 checks passed
@jl-santana jl-santana deleted the fix/load-schema-from-registry branch January 8, 2024 15:03
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.

3 participants