Skip to content

feat: implement letter.lowercase and letter.uppercase #77

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

Merged
merged 7 commits into from
Oct 27, 2022

Conversation

ccjmne
Copy link
Contributor

@ccjmne ccjmne commented Oct 26, 2022

Pretty trivial implementation!

However, something is wonky in the changes to the typings tests I introduced in c2200a4...
The tests were broken by introducing lowercase and uppercase properties to letter, and when I tried reproducing a minimal example of it on typescriptlang.org, of course there, an Asdf<T> & { yes: 'no' } does extend Asdf<any>. I couldn't put my finger on what the fix should be.

There might be a better way to handle them through savvy typing of the extractRegExp function, but I couldn't figure it out myself. 🙇‍♀️

Resolves #37.

@what-the-diff
Copy link

what-the-diff bot commented Oct 26, 2022

  • Added letter.lowercase and letter.uppercase to the inputs
  • Updated tests for lowercase, uppercase and not versions of these new inputs

Copy link
Member

@danielroe danielroe left a comment

Choose a reason for hiding this comment

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

The test issues you encountered are flagging that the type generation is broken at the moment.

(To be clear, this is the most difficult part of the PR for the reason you've encountered.)

We might consider rethinking the API. Maybe letter.any, letter.lowercase, letter.uppercase as three in parallel? wdyt @didavid61202

@didavid61202
Copy link
Collaborator

didavid61202 commented Oct 27, 2022

HI @danielroe, @ccjmne . I've made a PR #79 related to the type inferencing issue encountered in the PR.
If #79 is merged, we can remove the type casting in test file and merge this PR 👍

@vercel
Copy link

vercel bot commented Oct 27, 2022

Someone is attempting to deploy a commit to a Personal Account owned by @danielroe on Vercel.

@danielroe first needs to authorize it.

@vercel
Copy link

vercel bot commented Oct 27, 2022

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

Name Status Preview Updated
magic-regexp ✅ Ready (Inspect) Visit Preview Oct 27, 2022 at 3:15PM (UTC)

Copy link
Member

@danielroe danielroe left a comment

Choose a reason for hiding this comment

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

Thank you both! ❤️

@danielroe danielroe changed the title Implement letter.lowercase and letter.uppercase feat: implement letter.lowercase and letter.uppercase Oct 27, 2022
@danielroe danielroe merged commit 70afa5b into unjs:main Oct 27, 2022
@ccjmne
Copy link
Contributor Author

ccjmne commented Oct 27, 2022

This was really nice!
I should get off my butt and try to participate more in some open-source projects outside of Oktoberfest as well. 🤯

I learned some things, thought about some implementation details, read code that's not mine, discovered the Vite bundler, realised that pnpm is really getting popular...
All of that, by doing nearly nothing! 😂

Great experience 👍
Also, delightful interactions with you guys 😊

Cheers!

@didavid61202
Copy link
Collaborator

@ccjmne Thanks!
I'm also on the same track, trying to participate more in open-source projects, and have learned quite a lot from the best developers/maintainers like Daniel, Anthony, and Pooya Parsa (All from Nuxt team!).
And I will highly recommend checking out Nuxt repo and unjs repos if you haven't 👍

Thanks for your contribution! 💚

Cheers! 🍻

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.

differentiate lowercase letter, uppercase letter, or combine them
3 participants