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.

Sign up for GitHub

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

Jump to bottom

feat(builder): add max min length in string option #8214

Merged
merged 1 commit into from
Jul 7, 2022
Merged

feat(builder): add max min length in string option #8214

merged 1 commit into from
Jul 7, 2022

Conversation

imranbarbhuiya
Copy link
Contributor

@imranbarbhuiya imranbarbhuiya commented Jul 2, 2022 •
edited by SpaceEEC
Loading

Please describe the changes this PR makes and why it should be merged:

Status and versioning classification:

  • Code changes have been tested against the Discord API, or there are no code changes
  • I know how to update typings and have done so, or typings don't need updating
  • This PR changes the library's interface (methods or parameters added)

@vercel
Copy link

vercel bot commented Jul 2, 2022 •
edited
Loading

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

1 Ignored Deployment
Name Status Preview Updated
discord-js ⬜️ Ignored (Inspect) Jul 6, 2022 at 10:50AM (UTC)

@Jiralite Jiralite added this to the builders v1 milestone Jul 2, 2022
@almeidx
Copy link
Member

almeidx commented Jul 2, 2022

Can you update the max length?

@imranbarbhuiya imranbarbhuiya force-pushed the feat/string-length-in-builder branch from e227ac3 to 49ea267 Compare July 3, 2022 15:36
@codecov-commenter
Copy link

codecov-commenter commented Jul 3, 2022 •
edited by codecov bot
Loading

Codecov Report

Merging #8214 (10d4f9e) into main (8198da5) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #8214      +/-   ##
==========================================
+ Coverage   91.01%   91.05%   +0.04%     
==========================================
  Files          85       85              
  Lines        6944     6976      +32     
  Branches     1039     1041       +2     
==========================================
+ Hits         6320     6352      +32     
  Misses        580      580              
  Partials       44       44
Flag Coverage Δ
builders 97.82% <100.00%> (+0.02%) ⬆️
collection 100.00% <ø> (ø)
proxy 79.83% <ø> (ø)
rest 92.31% <ø> (ø)
utilities 50.00% <ø> (ø)
voice 64.16% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...s/src/interactions/slashCommands/options/string.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8198da5...10d4f9e. Read the comment docs.

@iCrawl iCrawl requested a review from vladfrangu July 4, 2022 15:40
vladfrangu
vladfrangu requested changes Jul 4, 2022
View reviewed changes
@imranbarbhuiya imranbarbhuiya force-pushed the feat/string-length-in-builder branch from 49ea267 to ab079e8 Compare July 4, 2022 16:20
@imranbarbhuiya imranbarbhuiya requested a review from vladfrangu July 4, 2022 16:21
@iCrawl
Copy link
Member

iCrawl commented Jul 6, 2022

So, this might be a bit controversial here but, if you update -types in one place, we should probably update it everywhere, especially as long as v14 is not released yet.

@imranbarbhuiya
Copy link
Contributor Author

imranbarbhuiya commented Jul 6, 2022 •
edited
Loading

So, this might be a bit controversial here but, if you update -types in one place, we should probably update it everywhere, especially as long as v14 is not released yet.

I did. Didn't I? Also for /proxy #8214 (comment)

@imranbarbhuiya imranbarbhuiya force-pushed the feat/string-length-in-builder branch 2 times, most recently from a5201f6 to 5e249e2 Compare July 6, 2022 10:48
@imranbarbhuiya imranbarbhuiya force-pushed the feat/string-length-in-builder branch from 5e249e2 to 10d4f9e Compare July 6, 2022 10:50
@iCrawl
Copy link
Member

iCrawl commented Jul 6, 2022

Guess I am just surprised by the lack of breaking changes 😆
Thanks for double checking.

iCrawl
iCrawl approved these changes Jul 6, 2022
View reviewed changes
Copy link
Member

@iCrawl iCrawl left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM, blocked until upstream is merged.

@iCrawl iCrawl added the blocked label Jul 6, 2022
@SuperchupuDev
Copy link
Contributor

upstream pr was merged 🚀

@Jiralite Jiralite removed the blocked label Jul 7, 2022
@iCrawl iCrawl merged commit 96c8d21 into discordjs:main Jul 7, 2022
@imranbarbhuiya imranbarbhuiya deleted the feat/string-length-in-builder branch July 7, 2022 19:36
cyan-2048 pushed a commit to cyan-2048/discord.js that referenced this pull request May 8, 2024
@imranbarbhuiya
feat(builder): add max min length in string option (discordjs#8214)
7955ec0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jiralite Jiralite Jiralite requested changes

@suneettipirneni suneettipirneni suneettipirneni requested changes

@vladfrangu vladfrangu vladfrangu approved these changes

@iCrawl iCrawl iCrawl approved these changes

@SpaceEEC SpaceEEC SpaceEEC approved these changes

@kyranet kyranet Awaiting requested review from kyranet

Assignees
No one assigned
Labels
packages:builders packages:discord.js packages:rest packages:voice semver:minor
Projects
discord.js
Archived in project
Milestone
builders v1
Development

Successfully merging this pull request may close these issues.
9 participants
@imranbarbhuiya @almeidx @codecov-commenter @iCrawl @SuperchupuDev @vladfrangu @SpaceEEC @Jiralite @suneettipirneni