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

compiled with advance optimization #96

Open
wants to merge 38 commits into
base: master
Choose a base branch
from

Conversation

yathit
Copy link

@yathit yathit commented Aug 9, 2013

Add few param annotation and make some changes in object inherit pattern similar to closure library.

@tonistiigi
Copy link
Member

Thanks.

It will take me some time to properly review this. If you need something quicker then you can make other PRs with smaller chunks of code.

Some quick notes:

  • there's an .idea folder that is commited.
  • there' some history about a "typing game" under demos(duplicate of zlizer). Would be good to clean it out.
  • lots of commits are not correctly named. For example commit named "compiling" that contains comment fixes.
  • Actionmanager change is potentially breaking. You can't check instanceof any more.
  • In director: fps check must stay and also the default listener is needed for mobile. Have to check the transition changes.
  • Indention changes in EventDispatcher. New pull requests should use 4 space tabulation.
  • Whats the use case for droppablelabel. Seems like something that should be included in a game.
  • I don't think we need a logger property in transition class just to show the deprecation warning.

I really appreciate the work you've done and especially would like to get all the commenting fixes merged in but we need to clean this up. Also I can't accept API changes that would break some old code without a good reason.

Chunks that should be in separate commits(and ideally in separate pull requests):

  • Commenting fixes
  • Style fixes(indention, semicolons)
  • goog.base inheritance
  • Removing deprecated code
  • label fixes
  • ScheduleManager class
  • Properties as strings for advanced compilation
  • Changes in Node/director
  • Changes in events

@yathit
Copy link
Author

yathit commented Aug 12, 2013

Thanks for your very nice game engine and comments.

It was quick pull request to see if maintainer are interested in such changes. I will send a new pull request, but there are some breaking API changes, of which I could be not removed.

  • there's an .idea folder that is committed.

will remove

  • there' some history about a "typing game" under demos(duplicate of zlizer). Would be good to clean it out.

has been removed.

  • lots of commits are not correctly named. For example commit named "compiling" that contains comment fixes.
  • Actionmanager change is potentially breaking. You can't check instanceof any more.

No. instanceof method can be used.

  • In director: fps check must stay and also the default listener is needed for mobile. Have to check the transition changes.

OK, but I still think decouple with goog.DEBUG. perhaps, introduce lime.DEBUG.

  • Indention changes in EventDispatcher. New pull requests should use 4 space tabulation.
  • Whats the use case for droppablelabel. Seems like something that should be included in a game.

it is not mine.

  • I don't think we need a logger property in transition class just to show the deprecation warning.

OK

Regarding, coding style, I strictly follow Google JS guide and gslint. So it will be 2 space indenting and 80 char per line. The code must be compiled with the most restricted closure compiler setting.

Thanks again for great library.

@tonistiigi
Copy link
Member

History also needs to be cleaned so try to start from a new branch and cherry pick/squash.

4 spaces. This isn't up for discussion. I also use 2 spaces myself but theres nothing worse than reading a file that has has some lines using 2 spaces and some lines with 4 spaces. And we can't convert the whole project because it will screw up all merges from other branches.

@yathit
Copy link
Author

yathit commented Aug 13, 2013

of course, 4 spaces, for this project.

It will take a while to get new PR. I contribute open source projects on my spare time.

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