Refactor line ingredient pasting#772
Conversation
ClaudeMetz
left a comment
There was a problem hiding this comment.
Thanks for the PR. So there's a few things here I'm not sure about, I'll mention them and I'll have to sit down to think about them some more.
First, I don't like the passing a whole set of tags to :paste. Maybe it's unavoidable though. All the things that this adds though makes it not really much less of a mess than the fake object thing. It's conceptually cleaner, but this being quite a mess on Line:paste is why I thought about extending SimpleItem with a paste method. That would have its own mess though too for sure.
Second, the Recipe:set_temperature method. I agree that checking the compatibility is a good idea. I don't like making it a class method necessarily. If we do that, we should have a look at all similar situations and see if that method can be used there as well. Plus, there's a similar situation on Fuel, so that should get its own method too and be used wherever applicable. Generally, I'm not sure if a method to check compatibility would be better, and then setting it separately. The code currently just sets the .temperatures field without a method, which isn't ideal but if we add a way to do it through a method, it needs to be changed to that everywhere.
Okay that's just some thoughts, if that makes you want to change anything feel free, otherwise I'll get back to this and think through it on my own. This is the part of reviewing PRs I struggle with, if it's not as I'd expect it, it takes me a lot of energy to get over the hump of thinking through it myself. Not your fault at all, just explaining it from my perspective.
|
We can split Extending |
|
Another option is to just make SimpleItem into proper objects. The reason I didn't is that they are just fully determined by the solver, and objects are usually something that you want to keep around, carrying some data the user has influence over. It does make it awkward to handle them alongside objects though, which is what all this is about. So all that is why making it a pseudo-object 'feels right'. The issue with making it a full object is that it goes through a lot of unique IDs quite fast, as it uses dozens each factory solve. I don't know if that would actually be a problem, the limit is 2^53 for Lua 5.2. Probably not a limit someone would ever get to? |
|
We could reuse the indices, but it will make allocating one But perhaps it's not necessary at all. I expect that just implementing But we should definitely move the paste logic from |
|
Yes, agreed on all counts I think. |
Move pasting logic from
production_handler.handle_item_clicktoLine.paste.Still not ideal, because the target is not the actual line box, but it's better IMO than extending
SimpleItemwith apastefunction.Now also checks whether the copied temperature is valid for the target recipe, and only pastes it if it is.