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

new Lua module: enable-util #4401

Closed
wants to merge 1 commit into from
Closed

Conversation

chdoc
Copy link
Contributor

@chdoc chdoc commented Mar 24, 2024

This is obviously not yet ready to be merged. However, I would like to know what you think of the idea, and what would you needed to merge this. Feel free to just edit this as you see fit.

Copy link
Member

@myk002 myk002 left a comment

Choose a reason for hiding this comment

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

reducing boilerplate is good. I'm having trouble seeing exactly how existing use cases would be served by this abstraction, though. It might be useful to prototype conversions of a set of representative scripts to be sure the API is going to serve their needs

return
end

if sc ~= SC_MAP_LOADED or df.global.gamemode ~= df.game_mode.DWARF then
Copy link
Member

Choose a reason for hiding this comment

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

especially as adventure mode is expected soon, this will need to become more flexible.

end

-- map was loaded, restore global data and reschedule
global_store = dfhack.persistent.getSiteData(global_key, {enabled=false})
Copy link
Member

Choose a reason for hiding this comment

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

this isn't going to affect the copy of global_store in the client script. maybe utils.assign, but you'd also have to manually clear out old contents before assigning to the table.


local repeatUtil = require('repeat-util')

function assignPersistentGlobal(global_key, global_store, key, value)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is flexible enough in practice. scripts may be modifying fields deep in the state table. The pattern I've usually seen is "make a batch of changes to the state, then persist from the top". However, with that pattern, this function reduces to just the call to dfhack.persistent.saveSiteData and the value of abstracting that out into an enable-util function becomes questionable

@myk002
Copy link
Member

myk002 commented May 16, 2024

closing this for now since I think the API isn't quite flexible enough for current script needs

@myk002 myk002 closed this May 16, 2024
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

2 participants