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

Adds '@type' tag support. #33

Closed
wants to merge 4 commits into from
Closed

Adds '@type' tag support. #33

wants to merge 4 commits into from

Conversation

iqrow
Copy link
Contributor

@iqrow iqrow commented Apr 23, 2014

No description provided.

@petebacondarwin
Copy link
Member

Great. Thanks @iqrow - could you also add a unit test along the lines of https://github.com/angular/dgeni-packages/blob/master/jsdoc/spec/tag-defs/tags.spec.js#L113-L122

@mary-poppins
Copy link

I'm sorry, but I wasn't able to verify your Contributor License Agreement (CLA) signature. CLA signature is required for any code contributions to any Angular repo.

Please sign our CLA and ensure that the CLA signature email address and the email address in this PR's commits match.

If you signed the CLA as a corporation, please let us know the company's name.

Thanks a bunch!

PS: If you signed the CLA in the past then most likely the email addresses don't match. Please sign the CLA again or update the email address in the commit of this PR.
PS2: If you are a Googler, please sign the CLA as well to simplify the CLA verification process.

@iqrow
Copy link
Contributor Author

iqrow commented Apr 24, 2014

@mary-poppins - apologies for my ignorance, didn't realise there was something I needed to sign. Have done now.

@petebacondarwin - I've written the unit test and I want to run it to ensure it's working before committing. I can't see a straightforward way to do that in the repo, please enlighten me.

Thanks.

@petebacondarwin
Copy link
Member

Try
npm test
On 24 Apr 2014 09:16, "IQrow" notifications@github.com wrote:

@mary-poppins https://github.com/mary-poppins - apologies for my
ignorance, didn't realise there was something I needed to sign. Have done
now.

@petebacondarwin https://github.com/petebacondarwin - I've written the
unit test and I want to run it to ensure it's working before committing. I
can't see a straightforward way to do that in the repo, please enlighten me.

Thanks.

Reply to this email directly or view it on GitHubhttps://github.com//pull/33#issuecomment-41254524
.

@mary-poppins
Copy link

CLA signature verified! Thank you!

Someone from the team will now triage your PR and it will be processed based on the determined priority (doc updates and fixes with tests are prioritized over other changes).

@iqrow
Copy link
Contributor Author

iqrow commented Apr 24, 2014

Thanks @petebacondarwin that worked, but I'm getting a lot of failing tests (not all of them though) so perhaps something isn't quite set up right here.

103 tests, 189 assertions, 71 failures, 0 skipped

If you have the time and patience, I'd appreciate help with why I'm getting a lot of TypeErrors when trying npm test:

TypeError: undefined is not a function

I don't know if this is a symptom of my environment of if the project has a load of failing tests.

In the meantime - I have pushed my spec, so that if you wanted to, you could merge and close this PR.

@petebacondarwin
Copy link
Member

I'll take a look!

@iqrow
Copy link
Contributor Author

iqrow commented Apr 24, 2014

Thanks.

@petebacondarwin
Copy link
Member

OK, the failed tests were because I had not updated the package.json to use the newer version of dgeni. I have pushed a commit to master to fix this. Now the tests are passing on master.

Now let's take a look at your test :-)

describe("type", function() {

it("should transform into a type object", function() {
var doc = parseDoc("@type {string} varOfTypeString description of var");
Copy link
Member

Choose a reason for hiding this comment

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

This is why it is good to have tests! If we look at the jsdoc definition of @type you can see it only allows a type and not a name or a description: http://usejsdoc.org/tags-type.html

So this line can only be:

var doc = parseDoc("@type {string}");

If you are documenting a var then you would probably use a combination of tags such as:

/**
 * @name varOfTypeString
 * @type {string}
 * @description description of var
 */
var varOfTypeString = 'hello';

In fact the next version of the jsdoc package can actually read the name from the code, so you could get away with:

/**

@petebacondarwin
Copy link
Member

Thanks @iqrow for this. It is great to have a new contributor!

@iqrow
Copy link
Contributor Author

iqrow commented Apr 24, 2014

Well I'll be. learns something new

Thanks very much! Feels good to contribute! :)

Now to use this in our own project, I'm going to have to copy these changes into our version of dgeni-packages until you release a new version on npm right? Or can I just do npm update dgeni-packages and get these changes into our project?

@petebacondarwin
Copy link
Member

You'll need to copy it for now. We need to do a bit more work on
dgeni-packages@master before we can release 0.9.0

On 24 April 2014 11:23, IQrow notifications@github.com wrote:

Well I'll be. learns something new

Thanks very much! Feels good to contribute! :)

Now to use this in our own project, I'm going to have to copy these
changes into our version of dgeni-packages until you release a new version
on npm right? Or can I just do npm update dgeni-packages and get these
changes into our project?

Reply to this email directly or view it on GitHubhttps://github.com//pull/33#issuecomment-41264673
.

@iqrow
Copy link
Contributor Author

iqrow commented Apr 24, 2014

Cool, thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants