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

changed the formspec method #751

Closed
wants to merge 1 commit into from

Conversation

pinkysnowman
Copy link
Contributor

switched to table.concat() for faster loading

switched to table.concat() for faster loading
@kaeza
Copy link
Contributor

kaeza commented Dec 22, 2015

Conflicts with #736, and is not needed for an operation that is not executed very often.

@asl97
Copy link

asl97 commented Dec 22, 2015

table.concat is slower than joining a bunch of string without needing to store them in a variable

unlike the sethome, the amount of args doesn't grow ether, it will always be the same amount of args

there no reason for this, this is slower than the current code.

@pinkysnowman
Copy link
Contributor Author

pinkysnowman commented Dec 22, 2015 via email

@kilbith
Copy link
Contributor

kilbith commented Dec 22, 2015

👎 .. is already a concatenation meant for plain strings, you're basically using needlessly more ressources with table.concat (which is useful to use when your values are necessarily in an array and that is likely to be dynamically redefined).

The best way for optimizing this is to avoid the concatenation at all, i.e. make a long, continuous string (but you lost the readability).

Also rather bad-style deontologically to open a PR conflicting short after another.

@pinkysnowman
Copy link
Contributor Author

pinkysnowman commented Dec 22, 2015 via email

@asl97
Copy link

asl97 commented Dec 23, 2015

@pinkysnowman now that is just rude, what kilbith said is valid

@kilbith

  • if 'newline' doesn't effect formspec, [[ and ]] could be use instead of long continuous string.
  • pinkysnowman might not have known about that PR

@est31
Copy link
Contributor

est31 commented Dec 25, 2015

I've removed a particularly rude comment.

@0-afflatus
Copy link

I have to say I find use of terms like 'deontologically' somewhat egregious.

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.

None yet

6 participants