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

More readable options.txt #399

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

Conversation

WilliamFr0g
Copy link
Contributor

qixils
qixils previously requested changes May 25, 2022
Copy link
Collaborator

@qixils qixils left a comment

Choose a reason for hiding this comment

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

same concern as previous review

main.lua Outdated Show resolved Hide resolved
@alesan99 alesan99 added the change An existing feature should be tweaked label May 29, 2022
Now not horrible inefficient
@WilliamFr0g
Copy link
Contributor Author

Updated to optimize saving code because it was very inefficient and concatenation-heavy

Copy link
Collaborator

@qixils qixils left a comment

Choose a reason for hiding this comment

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

Updated to LÖVE 11, and resolved the criticism of my earlier review.

I have to say there's a lot of options for how to improve the behemoth that is config saving and loading, but I don't really think a migration to string formatting is one of them. To me it feels even less readable than before and that's quite saying something. It strikes me as an especially premature optimization, as these functions realistically will only take a few milliseconds at most to run and are hardly ever called, and I'm not especially convinced that string formatting would even be all that much faster in a context where all the args are going to be computed regardless. I'd personally prefer if this was reverted back to just a simple \n addition, or upgraded to something like a proper table where keys are set then converted to text at the end (like is done for some of the individual keys).

@qixils qixils dismissed their stale review August 12, 2024 22:29

Newline issue resolved

@WilliamFr0g
Copy link
Contributor Author

Yeah the main change was to make it more readable, the changes to the saving was mostly because I was bored lol. Saving the config can take a tangible amount of time if you accidentally travel to a very high numbered world and it saves a few hundred or thousand reachedworlds entries, but I think that's a separate problem

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
change An existing feature should be tweaked
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants