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

enhance(ui): add better error messages for duplicate key in config.edn #8488

Merged
merged 3 commits into from Mar 2, 2023

Conversation

sallto
Copy link
Contributor

@sallto sallto commented Feb 1, 2023

fixes #8419
grafik

@logseq-cldwalker
Copy link
Collaborator

@sallto I can take a look at this once tests are passing. Putting this on hold in the meantime

@logseq-cldwalker logseq-cldwalker added the :type/hold Hold this PR. won't merge for now label Feb 1, 2023
@andelf
Copy link
Collaborator

andelf commented Feb 14, 2023

Many thanks.
I'm planning to integrate this into the file editor directly like #8584

See-also: https://codemirror.net/5/demo/lint.html

@Bad3r
Copy link
Collaborator

Bad3r commented Feb 14, 2023

I'm planning to integrate this into the file editor directly like #8584

The error message is still useful for when users edit config.edn outside of Logseq

@sallto
Copy link
Contributor Author

sallto commented Feb 25, 2023

I'm sorry for not getting back to you sooner. I was very busy with University. CI is passing now.

@Bad3r
Copy link
Collaborator

Bad3r commented Feb 26, 2023

@sallto no worries that takes priority. Good luck!

@logseq-cldwalker
Copy link
Collaborator

@sallto No worries. Could you add a test for this error like the other error messages in this file? See frontend.handler.common.config-edn-test

@logseq-cldwalker logseq-cldwalker added awaiting-response Issue will be closed if a reply is not received and removed :type/hold Hold this PR. won't merge for now labels Feb 27, 2023
@logseq-cldwalker logseq-cldwalker removed the awaiting-response Issue will be closed if a reply is not received label Mar 1, 2023
Copy link
Collaborator

@logseq-cldwalker logseq-cldwalker left a comment

Choose a reason for hiding this comment

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

@sallto Thanks for the error message and test! 👍 🚢 Works great

@logseq-cldwalker logseq-cldwalker merged commit fd3afac into logseq:master Mar 2, 2023
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.

Config validation; duplicate key wrong error
4 participants