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

added suman #19817

Closed
wants to merge 9 commits into from
Closed

Conversation

ORESoftware
Copy link
Contributor

Please fill in this template.

  • 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 npm run lint package-name (or tsc if no tslint.json is present).

Select one of these and delete the others:

If adding a new definition:

  • If this is for an NPM package, match the name. If not, do not conflict with the name of an NPM package.

@dt-bot
Copy link
Member

dt-bot commented Sep 18, 2017

types/suman/index.d.ts

Checklist


types/suman/suman-tests.d.ts

Checklist

@ORESoftware
Copy link
Contributor Author

Ok, so what does this mean:

Error: Directory /home/travis/build/DefinitelyTyped/DefinitelyTyped/types/suman has unused file suman-tests.d.ts
    at /home/travis/build/DefinitelyTyped/DefinitelyTyped/node_modules/types-publisher/src/lib/definition-parser.ts:365:11
    at Generator.next (<anonymous>)
    at fulfilled (/home/travis/build/DefinitelyTyped/DefinitelyTyped/node_modules/types-publisher/bin/lib/definition-parser.js:4:58)
    at <anonymous>

the test file is basically empty, because I don't really need tests for my typings.

@typescript-bot typescript-bot added New Definition This PR creates a new definition package. The Travis CI build failed labels Sep 19, 2017
@typescript-bot
Copy link
Contributor

@ORESoftware Please fix the failures indicated in the Travis CI log.

@mhegazy
Copy link
Contributor

mhegazy commented Sep 25, 2017

@ORESoftware, please address the lint failures.

Copy link
Contributor Author

@ORESoftware ORESoftware left a comment

Choose a reason for hiding this comment

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

Looks good, I don't know why "declaration:false" is not good enough, I need to remove that entire line? Seems unnecessary?

@@ -0,0 +1,47 @@
// Type definitions for suman 1.1
Copy link

Choose a reason for hiding this comment

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

It doesn't look like you changed anything about the template besides adding the package name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will do anything to get this to work, frankly I am lost, need help.

Copy link

Choose a reason for hiding this comment

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

It looks like suman provides its own typings in lib/index.d.ts. You shouldn't need to add these to DefinitelyTyped when they are already bundled with the package.

@@ -0,0 +1,3 @@

Copy link

Choose a reason for hiding this comment

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

It will need tests to ensure that code like https://github.com/sumanjs/suman#simple-example works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is just a dummy PR, so that I know what to expect. For example, by submitting this PR, I have learned that I cannot use I as the first character of an interface name... so this is an IMPORTANT QUESTION: Can I really not name any interface starting with an I? As in interface IFooBar {}... that is a really restrictive and unexpected limitation that forces me to refactor a lot of code :(

@RyanCavanaugh
Copy link
Member

@ORESoftware Please fix the failures indicated in the Travis CI log.

@ORESoftware
Copy link
Contributor Author

will do thx

@ORESoftware
Copy link
Contributor Author

@RyanCavanaugh @andy-ms

I finally got the build to pass with TravisCI.

I am really loving the tslint tool, it's a great way to create consistent projects in the @types ecosystem.

I am having trouble getting tslint to work locally, if you can answer this, that would make my day better:
palantir/tslint#3430

@RyanCavanaugh
Copy link
Member

@andy-ms - Thanks for your review of this PR! Can you please look at the new code and update your review status if appropriate?

@RyanCavanaugh RyanCavanaugh moved this from New Definitions to Merge: YSYL in Pull Request Triage Backlog Nov 6, 2017
@RyanCavanaugh RyanCavanaugh added the Unmerged The author did not merge the PR when it was ready. label Nov 6, 2017
@RyanCavanaugh
Copy link
Member

This PR has been open and unchanged 5 days without signoff or complaint. This will be merged by a maintainer soon if there are no objections.

@ORESoftware
Copy link
Contributor Author

ORESoftware commented Nov 6, 2017

SGTM thanks - I will make note of this issue history to make things smoother next time.

@ghost
Copy link

ghost commented Nov 6, 2017

Closing as suman provides its own types.

@ghost ghost closed this Nov 6, 2017
@RyanCavanaugh RyanCavanaugh removed this from Merge: YSYL in Pull Request Triage Backlog Nov 6, 2017
@ORESoftware
Copy link
Contributor Author

ORESoftware commented Nov 6, 2017

@andy-ms, suman does NOT provide all its own types, not even close. This project, suman-types provides the types that are not included in suman itself.

I essentially want to publish a git subtree of suman-types to @types/suman.

I use the suman-types project when developing locally. I suppose I could abandon the suman-types project and just use the DefinitelyTyped project directly. But either way, I am going to want to publish to @types/suman.

@ghost
Copy link

ghost commented Nov 6, 2017

Why not make a pull request to suman adding the missing types?

@ORESoftware
Copy link
Contributor Author

ORESoftware commented Nov 6, 2017

@andy-ms the sumanjs ecosystem is made up of 10+ projects. The dependency graph of that ecosystem is complex. The reason why I don't want to include all the types in suman itself, is that in some cases I do not want to npm link the suman project to certain projects locally, instead I wish to npm linkthe suman-types project because it's lighter-weight - just the typings.... So suman-types serves as a lighter-weight shared dependency of just the types for the whole sumanjs ecosystem.

@ORESoftware
Copy link
Contributor Author

ORESoftware commented Nov 6, 2017

But it seems like I should bypass @types....I can include suman-types in the suman project, and then export the types from there. This is all very confusing but one thing is becoming more certain...making PRs and waiting for PRs to @types is a little bit stressful and the loop is slow. Will close.

@ghost
Copy link

ghost commented Nov 6, 2017

Yes, DefinitelyTyped is just for packages that didn't come bundled with their own types -- if this is your own project you should include types yourself.

@ORESoftware
Copy link
Contributor Author

yeah np, it's a little confusing, but like I said I can include suman-types in my suman project and then export the types from suman-types from suman. Thanks anyway.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
New Definition This PR creates a new definition package. Unmerged The author did not merge the PR when it was ready.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants