bahrmichael / factorio-tycoon

GNU General Public License v3.0
8 stars 6 forks source link

some cherry-picks: warnings, reveal option, surface in gps #316

Closed winex closed 2 months ago

winex commented 2 months ago

@bahrmichael , this is low priority when you have spare time please review these cherry-picks one-by-one from latest winex:rbgen

i thought some of these commits could be merged early (for translations, options, etc) to minimize later PR(s). so, please select:

  1. which ones you can accept at this stage
  2. which should i remove from here
  3. what text/description needs to be changed, reworded, etc...

tests

i've done some real tests with mods to see how things work and how we should handle some new cases like:

as i have many saves with different dev states now, i always do my tests against all of them. for ex: fix: migrate old TagsQueue entries that were w/o multi-surface are to be found only in our dev saves. it's more like an example code, but didn't do any harm.

migrations spam

i suppose all old migrations should be dropped at some later point when we get more stable like v0.8 or even v0.9.9. we can inform old-save-users that some previous version is needed to be ran first to correctly upgrade if players really want it that way.

thanks!

bahrmichael commented 2 months ago

i suppose all old migrations should be dropped at some later point when we get more stable like v0.8 or even v0.9.9. we can inform old-save-users that some previous version is needed to be ran first to correctly upgrade if players really want it that way.

Agreed, I'm fine with deprecating older migrations. Maybe removing migrations that are older than 6 months is a good start?

bahrmichael commented 2 months ago

as i have many saves with different dev states now, i always do my tests against all of them.

That's awesome! The next version will probably be a breaking change, so don't worry about that too much.

bahrmichael commented 2 months ago

With https://github.com/bahrmichael/factorio-tycoon/pull/316/commits/f6cbabe762a8f54603a4cd1ad60bbbdad839b9ef I'm rather leaning towards disabling the reveal by default. Do you have strong reasons for enabling it by default?

bahrmichael commented 2 months ago

With https://github.com/bahrmichael/factorio-tycoon/pull/316/commits/03910a6760eae2d299a320063f6872ad43c15ee6 I would much rather keep the longer variables, and not reuse them for different objects. Is it okay if we drop that change?

bahrmichael commented 2 months ago

I'm happy with the other changes :) Will wait on your feedback for the comments above @winex.

winex commented 2 months ago

With 03910a6 I would much rather keep the longer variables, and not reuse them for different objects. Is it okay if we drop that change?

yep, of course. i just added that var in some earlier PR on top of this if-blocks and changed some so it doesn't look that ugly for return checks being on random columns inside such condition checks. dropping...

With f6cbabe I'm rather leaning towards disabling the reveal by default. Do you have strong reasons for enabling it by default?

nope, it just helped to find those when playtesting. changing...

winex commented 2 months ago

rebased with: