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

Function 'pick' returns is documented to always return a value. #14320

Merged
merged 3 commits into from
Feb 1, 2017

Conversation

aredfox
Copy link
Contributor

@aredfox aredfox commented Jan 28, 2017

Function 'pick' returns is documented to always return a value. Before it's return type was :void, I'm sending this proposal as :any would fix this issue.
Reference to dot-object documentation: https://github.com/rhalff/dot-object#pickremove-a-value-using-dot-notation

Please fill in this template.

  • Make your PR against the master branch.
  • Use a meaningful title for the pull request. Include the name of the package modified.
  • Test the change in your own code. (Compile and run.)
  • Follow the advice from the readme.
  • Avoid common mistakes.
  • Run tsc without errors.
  • Run npm run lint package-name if a tslint.json is present.

Select one of these and delete the others:

If adding a new definition:

  • The package does not provide its own types, and you can not add them.
  • If this is for an NPM package, match the name. If not, do not conflict with the name of an NPM package.
  • Create it with npm run new-package package-name, not by basing it on an existing project.
  • tslint.json should be present, and tsconfig.json should have noImplicitAny, noImplicitThis, and strictNullChecks set to true.

If changing an existing definition:

  • Provide a URL to documentation or source code which provides context for the suggested changes: <>
  • Increase the version number in the header if appropriate.
  • If you are making substantial changes, consider adding a tslint.json containing { "extends": "../tslint.json" }.

If removing a declaration:

  • If a package was never on DefinitelyTyped, you don't need to do anything. (If you wrote a package and provided types, you don't need to register it with us.)
  • Delete the package's directory.
  • Add it to notNeededPackages.json.

- See issue DefinitelyTyped#14319

Function 'pick' returns is documented to always return a value. Before it's return type was :void, I'm sending this proposal as :any would fix this issue.
Reference to dot-object documentation: https://github.com/rhalff/dot-object#pickremove-a-value-using-dot-notation
@dt-bot
Copy link
Member

dt-bot commented Jan 28, 2017

dot-object/index.d.ts

to author (@nkovacic). Could you review this PR?
👍 or 👎?

Checklist

  • pass the Travis CI test?

@aredfox
Copy link
Contributor Author

aredfox commented Jan 28, 2017

URL to documentation & info: #14319

@aredfox
Copy link
Contributor Author

aredfox commented Jan 28, 2017

cc @rhalff - author of dot-object - for his advice/input

@@ -109,7 +109,7 @@ declare namespace DotObject {
* @param {Object} obj
* @param {Boolean} remove
*/
pick(path: string, obj: any, remove?: boolean): void;
pick(path: string, obj: any, remove?: boolean): any;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link

Choose a reason for hiding this comment

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

The return type is definitely any.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for the quick feedback @rhalff

Bumped version number as asked in checklist
@@ -1,4 +1,4 @@
// Type definitions for Dot-Object v1.4.2
// Type definitions for Dot-Object v1.4.3
Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • Bumped the version number, as asked in the checklist/guidelines

Copy link
Member

Choose a reason for hiding this comment

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

We only use MAJOR.MINOR for version numbers. Can you strip the last portion off?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, will make it 1.5 then I guess?

@@ -1,4 +1,4 @@
// Type definitions for Dot-Object v1.4.2
// Type definitions for Dot-Object v1.4.3
Copy link
Member

Choose a reason for hiding this comment

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

We only use MAJOR.MINOR for version numbers. Can you strip the last portion off?

To comply with maj.min versioning
ref comment DefinitelyTyped#14320 (comment)
@@ -1,4 +1,4 @@
// Type definitions for Dot-Object v1.4.2
// Type definitions for Dot-Object v1.5
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@DanielRosenwasser bumped it now straight to v1.5
thanks

@aredfox
Copy link
Contributor Author

aredfox commented Jan 29, 2017

is there a way I can work around this in typescript "typings" of "tsconfig.json" (in stead of editing the index.d.ts file every time I do an npm i on any machines where I and my fellow people work on) - until @DanielRosenwasser or @nkovacic reviewed and/or accepted this?

  • as I'm fairly new to typescript and I don't see an option to point it temporarily to a git repo/fork? Stupid question maybe?

aredfox added a commit to aredfox/electron-starter that referenced this pull request Jan 29, 2017
- added info on workaround for current @types/dot-object pick method (see PR: DefinitelyTyped/DefinitelyTyped#14320)
@nkovacic
Copy link
Contributor

👍 Looks good

@aredfox
Copy link
Contributor Author

aredfox commented Feb 1, 2017

A polite ping/bump to @DanielRosenwasser and @nkovacic to futher review/merge the PR when appropriate?

@DanielRosenwasser DanielRosenwasser merged commit dba28e9 into DefinitelyTyped:master Feb 1, 2017
@DanielRosenwasser
Copy link
Member

Thanks! Sorry for the delay!

To answer your question: right now you can take the .d.ts file and add it to a folder in your project. Then use path mappings (i.e. the paths field in your tsconfig.json) so that whenever someone imports a path like "foo", it always points to the appropriate .d.ts file

There's an extremely technical document about path mappings here: microsoft/TypeScript#5039

@aredfox
Copy link
Contributor Author

aredfox commented Feb 2, 2017

Thanks @DanielRosenwasser - no prob on the delay, I know we all have a job (or multiple) and this is all voluntary work. Thanks for introducting me to the path mappings as well!

@aredfox aredfox deleted the patch-1 branch February 2, 2017 07:37
annich-MS pushed a commit to annich-MS/DefinitelyTyped that referenced this pull request Feb 21, 2017
To comply with maj.min versioning
ref comment DefinitelyTyped#14320 (comment)
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.

None yet

5 participants