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

Some code style fixes #949

Closed
wants to merge 3 commits into from
Closed

Some code style fixes #949

wants to merge 3 commits into from

Conversation

Aliance
Copy link
Contributor

@Aliance Aliance commented Jul 28, 2015

wrong jsdoc

@@ -332,16 +325,16 @@ Crafty.c("Collision", {
* @see .checkHits
* @see .hit
*/
onHit: function (comp, callback, callbackOff) {
onHit: function (component, hit, noHit) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I actually liked the previous parameter names more, as the -Off part reflected their purpose better - function to be called after collision resolved.
Some suggestions: callbackOn/-Off, hitOn/-Off, etc.

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 agree, but PHPStorm highlights if function argument and phpdoc @param are different. May be change it in phpdoc?

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's update the documentation then accordingly

@mucaho
Copy link
Contributor

mucaho commented Jul 28, 2015

Looks good, have couple of suggestions.

@@ -78,17 +75,15 @@ Crafty.c("Collision", {
*
* @see Crafty.polygon
*/
collision: function (poly) {
collision: function (polygon) {
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 renamed this argument like it described here. You don't mind?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, this is good.

* checks for collisions.
* @param collisionData - Collision data object used to track collisions with
* the specified component.
/**
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've change /**@ to /** (removed the @) not /* because of IDE highlighting, as I can see, you documentation gets only /**@?

And I deleted #._createCollisionHandler and @comp Collision 'cause of its uselessness in private methods, also moved the description to the top of jsdoc.

@mucaho
Copy link
Contributor

mucaho commented Jul 30, 2015

This PR inspired me to open #951. We will hopefully fix code styles automatically soon, so I suggest you don't bother too much about indentation, as there are ~25 more files that need these fixes too :)

@Aliance
Copy link
Contributor Author

Aliance commented Jul 30, 2015

Ok, I see. Do I need to revert indentation changes?

@mucaho
Copy link
Contributor

mucaho commented Aug 15, 2015

Sorry for the long wait, I have been away from my working PC.
Did a final review, added some documentation code highlighting and merged with a6acb0d.

@mucaho mucaho closed this Aug 15, 2015
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.

2 participants