cameroncondry / cbc-kitten-scientists

Add-on for the wonderful incremental browser game: http://kittensgame.com/web/
MIT License
114 stars 94 forks source link

policy skip confirm #331

Closed PeterCheney closed 3 years ago

PeterCheney commented 3 years ago

new:

bug fixed:

Minor:

oliversalzburg commented 3 years ago

For automatically turning on reactors, the option in the UI should be renamed from "Turn on Steamworks" to "Turn on buildings" and this should probably be added to the README.

Some of the commits seem to be changing a lot more than the commit message suggests and it's not clear why some of those changes are necessary. This makes it very hard to assess if the changes are correct. If you could send smaller PRs, with less changes, that would make reviewing them a lot easier. Then we could also better document changes and possibly clarify the code.

I know that the existing code base and the way that some previous PRs have been reviewed did not set a great example, but it's become nearly impossible to judge the validity of code changes.

I can cherry-pick maybe 7ea3caa407d0b040bd9cac5860d11dcb56f0d53a from this branch, but, please, try to clarify the remaining changes either through code comments or the commit message. Ideally, if you clean up code, don't mix those changes into other changes that relate to the behavior of KS. This only makes it harder to review. Cleanup efforts are greatly appreciated, but, please, send them as separate PRs.

cameroncondry commented 3 years ago

I agree with @oliversalzburg here, there is too much in this PR to safely merge it straight into master. Overall the changes look good but I'm not sure about the bug fixes and how they will work long term.

@PeterCheney would you mind adding details about the issues this is fixing as well as a bit more information and use cases on how to reproduce the issues that this PR resolves? With some clearer details, we will be able to get this safely merged into master.

I really appreciate your interest and contributions to the script. Thank you!

oliversalzburg commented 3 years ago

@PeterCheney also sent me an email, asking me to review the PR and I requested some clarification. I feel the new commits make the changes a bit clearer.

Needless to say, future PRs should ideally only introduce a single change, so it can be discussed more easily.

I understand there is a bit of a language barrier and I'd hate to have that get in the way of having good changes merged into the code base. If there was only a single feature change or bugfix in the PR, it would be easier for other people (like myself) to add clarifying comments.

That being said, I'd be inclined to merge this PR if @cameroncondry also agrees it's fine.

cameroncondry commented 3 years ago

After some testing, I agree that this can be merged. We should keep an eye out in case some edge cases were missed. Thank you for the great additions!