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

Bugfix/fix in devicon.json #1485

Merged
merged 8 commits into from
Oct 29, 2022
Merged

Conversation

lunatic-fox
Copy link
Contributor

Double check these details before you open a PR

  • PR does not match another non-stale PR currently opened

Features

Removes unnecessary aliases of sequelize in devicon.json

This PR closes NONE

Notes

Copy link
Collaborator

@Snailedlt Snailedlt left a comment

Choose a reason for hiding this comment

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

That's a lot of commits for a small change, consider redoing the branch or rebasing on develop next time, so it's easier for reviewers to look through it :)

That said, nice catch 🕵️
Thanks for making a PR on it, very much appreciated 🙏

@Snailedlt
Copy link
Collaborator

@lunatic-fox
By the way, it shouldn't be possible for contributors to add the same aliases... even more reason to ship your feature request ASAP :) ref: #1481

@lunatic-fox
Copy link
Contributor Author

That's a lot of commits for a small change, consider redoing the branch or rebasing on develop next time, so it's easier for reviewers to look through it :)

That said, nice catch 🕵️
Thanks for making a PR on it, very much appreciated 🙏

I'll be doing that next time.
I got a little bit shocked by the amount of commits.

@Snailedlt
Copy link
Collaborator

@lunatic-fox hehe, yeah... In this case there was no issue, and it's rarely an issue for devicons since we do squash merging which squashes all of the commits into a single commit on the develop branch. However it could become an issue if there's a big feature, where you make multiple changes :) So just sort of a side note

@Snailedlt
Copy link
Collaborator

Btw, we should probably add this alias/base duplicate value check to our check-bot, so I made an issue (#1486) to make note of it :) I've done some resent changes to the check-bot myself, so I'll probably end up adding it myself

@Snailedlt Snailedlt merged commit a7fbb21 into devicons:develop Oct 29, 2022
@lunatic-fox
Copy link
Contributor Author

Btw, we should probably add this alias/base duplicate value check to our check-bot, so I made an issue (#1486) to make note of it :) I've done some resent changes to the check-bot myself, so I'll probably end up adding it myself

That's great!
I'm still learning how to work with bots, as soon as got more confident I'll help with that too. 🙂

@lunatic-fox lunatic-fox deleted the bugfix_sequelize branch June 1, 2023 12:29
@Snailedlt Snailedlt mentioned this pull request Feb 5, 2024
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