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

Fix to properly check for 'undefined' #684

Merged
merged 9 commits into from
Apr 25, 2013
Merged

Conversation

andr3nun3s
Copy link
Contributor

Still working on issue #89, just to make sure I'm on the right track as I've pinpointed every file in Source/Core that needs to be fixed.

@pjcozzi
Copy link
Contributor

pjcozzi commented Apr 24, 2013

@Andre-Nunes these look good, thanks. The only thing that sticks out to me is that the formatting for define([... you have is slightly different than what we use elsewhere.

You are welcome to keep this pull request open and continue to commit to your branch until you are ready for a larger review.

@andr3nun3s
Copy link
Contributor Author

Ok, I'll fix the formatting. I'll keep commiting untill all the files I've pinpointed are fixed.

@andr3nun3s
Copy link
Contributor Author

Ok, I'm done with Core/, I've noticed #694 so I guess I should hold off on this issue until both pull requests are sorted.

Let me know if something comes up.

@mramato
Copy link
Contributor

mramato commented Apr 25, 2013

Thanks a lot @Andre-Nunes! This cleanup is long overdue and it's nice to see you taking charge of it.

mramato added a commit that referenced this pull request Apr 25, 2013
Fix to properly check for 'undefined'
@mramato mramato merged commit 34e46e8 into CesiumGS:master Apr 25, 2013
@andr3nun3s andr3nun3s deleted the issue89 branch April 25, 2013 16:14
@mramato
Copy link
Contributor

mramato commented Apr 25, 2013

@Andre-Nunes I don't know if you noticed my mailing list email, but it turns our there were still some problems with your branch that I didn't pick up. I fixed them (since I was the one that took them into master before they were ready) but I wanted to provide you some additional feedback anyway.

  1. Before you open a pull request, always run ant jsHint on the command line. This should spit out any coding errors that you might have missed.
  2. Always run the full unit tests and make sure there are no failures. If there are failures, compare them with the same tests running in Cesium master to determine if the failures are specific to your branch or not.

Anyway, thanks again for the changes and I'm sorry for the confusion! I look forward to more pull requests from you in the future.

@andr3nun3s
Copy link
Contributor Author

Thanks for being so cool about it. I will keep that in mind for the future!

@pjcozzi
Copy link
Contributor

pjcozzi commented Apr 25, 2013

Thanks @Andre-Nunes. These are good changes, and many are complimentary to #694.

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.

3 participants