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

[@types/which] Update the options of which for version 3.0.0. #69576

Merged
merged 1 commit into from
May 30, 2024

Conversation

vthemelis
Copy link
Contributor

@vthemelis vthemelis commented May 11, 2024

Add nothrow option to both the sync and async APIs and add the missing delimiter option.

These have been available on the package since version 3.0.0: npm/node-which@8b0187c

Please fill in this template.

Select one of these and delete the others:

If changing an existing definition:

  • Provide a URL to documentation or source code which provides context for the suggested changes: https://github.com/npm/node-which/blob/main/lib/index.js
  • If this PR brings the type definitions up to date with a new version of the JS library, update the version number in the package.json.

@typescript-bot
Copy link
Contributor

typescript-bot commented May 11, 2024

@vthemelis Thank you for submitting this PR! I see this is your first time submitting to DefinitelyTyped 👋 — I'm the local bot who will help you through the process of getting things through.

This is a live comment which I will keep updated.

1 package in this PR

Code Reviews

Because you edited one package and updated the tests (👏), I can help you merge this PR once someone else signs off on it.

You can test the changes of this PR in the Playground.

Status

  • ✅ No merge conflicts
  • ✅ Continuous integration tests have passed
  • ✅ Most recent commit is approved by type definition owners or DT maintainers

All of the items on the list are green. To merge, you need to post a comment including the string "Ready to merge" to bring in your changes.


Diagnostic Information: What the bot saw about this PR
{
  "type": "info",
  "now": "-",
  "pr_number": 69576,
  "author": "vthemelis",
  "headCommitOid": "3eec79d57f3988aa502f1431de299325ff20016f",
  "mergeBaseOid": "86953d1148f231c8f1c534e4a718ac4fa7888767",
  "lastPushDate": "2024-05-11T17:18:52.000Z",
  "lastActivityDate": "2024-05-30T17:24:16.000Z",
  "maintainerBlessed": "Waiting for Code Reviews",
  "mergeOfferDate": "2024-05-30T16:55:58.000Z",
  "mergeRequestDate": "2024-05-30T17:24:16.000Z",
  "mergeRequestUser": "vthemelis",
  "hasMergeConflict": false,
  "isFirstContribution": true,
  "tooManyFiles": false,
  "hugeChange": false,
  "popularityLevel": "Critical",
  "pkgInfo": [
    {
      "name": "which",
      "kind": "edit",
      "files": [
        {
          "path": "types/which/index.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/which/tsconfig.json",
          "kind": "package-meta-ok"
        },
        {
          "path": "types/which/which-tests.ts",
          "kind": "test"
        }
      ],
      "owners": [
        "vvakame",
        "cspotcode",
        "peterblazejewicz"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Critical"
    }
  ],
  "reviews": [
    {
      "type": "approved",
      "reviewer": "sheetalkamat",
      "date": "2024-05-30T16:55:21.000Z",
      "isMaintainer": true
    },
    {
      "type": "approved",
      "reviewer": "alvinsga",
      "date": "2024-05-24T09:06:03.000Z",
      "isMaintainer": false
    },
    {
      "type": "stale",
      "reviewer": "EduardoAC",
      "date": "2024-05-21T21:05:59.000Z",
      "abbrOid": "e28621b"
    }
  ],
  "mainBotCommentID": 2105959863,
  "ciResult": "pass"
}

@typescript-bot
Copy link
Contributor

🔔 @vvakame @cspotcode @peterblazejewicz — please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

@typescript-bot typescript-bot moved this from Waiting for Code Reviews to Needs Maintainer Review in New Pull Request Status Board May 11, 2024
@vthemelis
Copy link
Contributor Author

The issue with this is that something like:

which('blah', { not_a_real_option: true })

typechecks fine (when it should probably not). Not totally sure how to avoid this just now. Would you recommend that I add more overloads instead?

@vthemelis vthemelis changed the title [which] Type-safe no-throw variant of the async which. [@types/which] Type-safe no-throw variant of the async which. May 11, 2024
Copy link
Contributor

@EduardoAC EduardoAC left a comment

Choose a reason for hiding this comment

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

LGTM

@typescript-bot typescript-bot added the Other Approved This PR was reviewed and signed-off by a community member. label May 11, 2024
@typescript-bot typescript-bot removed the Other Approved This PR was reviewed and signed-off by a community member. label May 16, 2024
@typescript-bot typescript-bot moved this from Needs Maintainer Review to Waiting for Code Reviews in New Pull Request Status Board May 16, 2024
@vthemelis
Copy link
Contributor Author

@vvakame, could you have a look? I think that there is parity in terms of what does and doesn't compiler now with the previous version (excluding the intentionally introduced new behaviour).

@typescript-bot typescript-bot moved this from Waiting for Code Reviews to Needs Maintainer Review in New Pull Request Status Board May 16, 2024
@typescript-bot
Copy link
Contributor

@EduardoAC Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review?

@vthemelis
Copy link
Contributor Author

Cc @EduardoAC @peterblazejewicz

@typescript-bot typescript-bot moved this from Needs Maintainer Review to Waiting for Code Reviews in New Pull Request Status Board May 17, 2024
@vthemelis vthemelis requested a review from EduardoAC May 17, 2024 20:46
@typescript-bot typescript-bot moved this from Waiting for Code Reviews to Needs Maintainer Review in New Pull Request Status Board May 17, 2024
@typescript-bot typescript-bot moved this from Needs Maintainer Review to Waiting for Code Reviews in New Pull Request Status Board May 17, 2024
@typescript-bot typescript-bot moved this from Waiting for Code Reviews to Needs Maintainer Review in New Pull Request Status Board May 17, 2024
@vthemelis vthemelis changed the title [@types/which] Type-safe no-throw variant of the async which. [@types/which] Update the options of which for version 3.0.0. May 17, 2024
@vthemelis
Copy link
Contributor Author

CC @cspotcode

@typescript-bot typescript-bot moved this from Needs Maintainer Review to Waiting for Code Reviews in New Pull Request Status Board May 18, 2024
@typescript-bot typescript-bot moved this from Waiting for Code Reviews to Needs Maintainer Review in New Pull Request Status Board May 18, 2024
@vthemelis
Copy link
Contributor Author

@EduardoAC can I have another review? I pushed some more changes to completely address typing for the latest version of the package.

// nothrow not specified
: TRet;

type AddArray<TOptions, TRet> = TOptions extends { all: infer TVal }
Copy link
Contributor

@EduardoAC EduardoAC May 21, 2024

Choose a reason for hiding this comment

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

Can you expand on the naming for AddNull and AdddArray? I see that the new type a bit obscure to understand what it represents.

When i see the code feels that you are more appending or transforming the original types

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, happy to do that. What name would work best? Maybe NullableIfNoThrow and ArrayIfAll ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, happy to do that. What name would work best? Maybe NullableIfNoThrow and ArrayIfAll ?

I was thinking something quite similar. Something in linesAppendNullIfNothrow and TransformToArrayIfAll. What are your thoughts? If you prefer yours, i will accept them as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, will amend later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

Copy link
Contributor

Choose a reason for hiding this comment

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

Well done, i appreciated the improvements

* Add `nothrow` option to both the sync and async APIs
* Add the missing `delimiter` option
* Refine typing when `all` or `nothrow` are specified but it can't be
  inferred if they are specified to `true` or `false` but a general
  `boolean`.

These have been available on the package since version 3.0.0: npm/node-which@8b0187c
@typescript-bot typescript-bot moved this from Needs Maintainer Review to Waiting for Code Reviews in New Pull Request Status Board May 21, 2024
@typescript-bot
Copy link
Contributor

@EduardoAC Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review?

@typescript-bot typescript-bot moved this from Waiting for Code Reviews to Needs Maintainer Review in New Pull Request Status Board May 21, 2024
Copy link
Contributor

@EduardoAC EduardoAC left a comment

Choose a reason for hiding this comment

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

LGTM

@typescript-bot typescript-bot added Other Approved This PR was reviewed and signed-off by a community member. and removed Other Approved This PR was reviewed and signed-off by a community member. labels May 21, 2024
@vthemelis
Copy link
Contributor Author

Thanks for the review @EduardoAC ! Do I need to get another review before merging?

@EduardoAC
Copy link
Contributor

Thanks for the review @EduardoAC ! Do I need to get another review before merging?

Usually, the merge is done by the repository owner, but if you can merge, go ahead. LGTM

@typescript-bot typescript-bot added the Unreviewed No one showed up to review this PR, so it'll be reviewed by a DT maintainer. label May 22, 2024
@typescript-bot
Copy link
Contributor

Re-ping @vvakame, @cspotcode, @peterblazejewicz:

This PR has been out for over a week, yet I haven't seen any reviews.

Could someone please give it some attention? Thanks!

@typescript-bot typescript-bot added the Other Approved This PR was reviewed and signed-off by a community member. label May 24, 2024
@sheetalkamat sheetalkamat moved this from Needs Maintainer Review to Waiting for Code Reviews in New Pull Request Status Board May 28, 2024
@typescript-bot
Copy link
Contributor

It has been more than two weeks and this PR still has no reviews.

I'll bump it to the DT maintainer queue. Thank you for your patience, @vthemelis.

(Ping @vvakame, @cspotcode, @peterblazejewicz.)

@typescript-bot typescript-bot moved this from Waiting for Code Reviews to Needs Maintainer Action in New Pull Request Status Board May 29, 2024
@typescript-bot typescript-bot added Maintainer Approved Self Merge This PR can now be self-merged by the PR author or an owner and removed Unreviewed No one showed up to review this PR, so it'll be reviewed by a DT maintainer. labels May 30, 2024
@typescript-bot typescript-bot moved this from Needs Maintainer Action to Waiting for Author to Merge in New Pull Request Status Board May 30, 2024
@typescript-bot
Copy link
Contributor

@vthemelis: Everything looks good here. I am ready to merge this PR (at 3eec79d) on your behalf whenever you think it's ready.

If you'd like that to happen, please post a comment saying:

Ready to merge

and I'll merge this PR almost instantly. Thanks for helping out! ❤️

(@vvakame, @cspotcode, @peterblazejewicz: you can do this too.)

@vthemelis
Copy link
Contributor Author

Ready to merge

@typescript-bot typescript-bot moved this from Waiting for Author to Merge to Recently Merged in New Pull Request Status Board May 30, 2024
@typescript-bot typescript-bot merged commit d49a96d into DefinitelyTyped:master May 30, 2024
4 checks passed
@typescript-bot typescript-bot removed this from Recently Merged in New Pull Request Status Board May 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Critical package Maintainer Approved Other Approved This PR was reviewed and signed-off by a community member. Self Merge This PR can now be self-merged by the PR author or an owner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants