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

✨ Add schema to gitmojis #1263

Merged
merged 8 commits into from
Jan 13, 2023
Merged

✨ Add schema to gitmojis #1263

merged 8 commits into from
Jan 13, 2023

Conversation

vhoyer
Copy link
Collaborator

@vhoyer vhoyer commented Jan 9, 2023

Description

Just read the commit messages, I go into detail on why I'm proposing those changes, if you want to discard the last commit (validation through API), and merging only the first one, I also wouldn't see any issue.

Linked issues

@vhoyer vhoyer requested a review from carloscuesta January 9, 2023 15:39
@vercel
Copy link

vercel bot commented Jan 9, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
gitmoji ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Jan 13, 2023 at 6:25PM (UTC)

vhoyer added 2 commits January 9, 2023 12:39
By using the $schema the editors can more easily provide insight on missing properties rather than waiting for the user to run the lint
This would be helpful in the scenario where someone uses the API to download the gitmojis information, and want to override something locally on their machine. The $schema would help make sure other tools like gitmoji-cli keep working with the local customizations included.
Copy link
Owner

@carloscuesta carloscuesta left a comment

Choose a reason for hiding this comment

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

Hey!

Good catch 👏🏼

packages/gitmojis/src/gitmojis.json Outdated Show resolved Hide resolved
packages/website/src/pages/api/gitmojis/index.ts Outdated Show resolved Hide resolved
@@ -4,7 +4,9 @@
"description": "An emoji guide for your commit messages.",
"main": "src/gitmojis.json",
"files": [
"src/gitmojis.json"
"src/gitmojis.d.ts",
Copy link
Owner

Choose a reason for hiding this comment

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

I think we can drop this as it's already specified in types:

Suggested change
"src/gitmojis.d.ts",

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I find it strange that we wouldn't need the types in the files array, because the types are declared on the package.json, but when we install the gitmojis pagackage outside the monorepo, the types file isn't present, which would lead to difficulties for other people trying to use the gitmoji type

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@carloscuesta and I think that's a problem, actually, should I open an issue about it, or can we keep using this thread to discuss this?

packages/website/src/pages/api/gitmojis/schema.ts Outdated Show resolved Hide resolved
@carloscuesta carloscuesta force-pushed the add-schema-to-gitmojis-json branch from 750fbb9 to cd340d3 Compare January 13, 2023 18:23
Copy link
Owner

@carloscuesta carloscuesta left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

@carloscuesta carloscuesta changed the title Add schema to gitmojis json ✨ Add schema to gitmojis Jan 13, 2023
@carloscuesta carloscuesta merged commit d2b5d6a into master Jan 13, 2023
@carloscuesta carloscuesta deleted the add-schema-to-gitmojis-json branch January 13, 2023 18:26
@vhoyer
Copy link
Collaborator Author

vhoyer commented Jan 13, 2023

thanks for the changes

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants