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

Builtin cleanup, part 1 (game). #1341

Closed
wants to merge 7 commits into from
Closed

Conversation

kaeza
Copy link
Contributor

@kaeza kaeza commented May 28, 2014

This does several things:

  • Breaks lines so each has an absolute maximum of 120 characters (tabs are taken as 4 spaces).
  • Fixes some variables being leaked to the global environment (Green Peace would be proud).
  • Optimizes some constructs.
  • Fixes some small(ish) bugs.
  • Most importantly: gets rid of features deprecated long ago.

@rubenwardy
Copy link
Member

This all looks good. +1

@ShadowNinja
Copy link
Member

Our official line length limit is 90 characters, and tabs are officially 8-space width.
Also, do_nodeupdate is pointless, you can just use nodeupdate directly (or so it seems...).
This will conflict with #1129, but looks good.

@sapier
Copy link
Contributor

sapier commented Jun 29, 2014

tab width is not specified ;-) can this be rebased to 0.4.10?

{x=-1,y=0,z=0},
{x=0,y=0,z=1},
{x=0,y=0,z=-1},
}
Copy link
Member

Choose a reason for hiding this comment

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

This can be moved above so that the tables are generated only on init. Or you can put the pos in here and avoid creating a separate table below. I'd use the first option though because it can be short-circuited.

Also, is there a reason that (0, 0, 0) isn't tried?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah good catch!

I'll move the table outside of the function.

I see no reason why it doesn't try (0, 0, 0) first. Guess it was a small mistake on part of the implementor.

@ShadowNinja ShadowNinja added Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements and removed Rebase needed The PR needs to be rebased by its author. labels Nov 18, 2014
@kaeza kaeza force-pushed the builtin_cleanup branch 2 times, most recently from 8de1826 to c2f3e43 Compare November 20, 2014 19:12
@ShadowNinja ShadowNinja added the Rebase needed The PR needs to be rebased by its author. label Nov 21, 2014
@kaeza
Copy link
Contributor Author

kaeza commented Nov 24, 2014

Will redo it later bit by bit.

@kaeza kaeza closed this Nov 24, 2014
@sfan5 sfan5 modified the milestone: 0.4.12 Dec 26, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements Rebase needed The PR needs to be rebased by its author. @ Script API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants