-
Notifications
You must be signed in to change notification settings - Fork 178
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
[script][inventory-manager]add vault book,storage book,pocket. adjust vault surface handling for container nesting data #6859
[script][inventory-manager]add vault book,storage book,pocket. adjust vault surface handling for container nesting data #6859
Conversation
inventory-manager.lic
Outdated
@@ -223,6 +240,20 @@ class InventoryManager | |||
else | |||
container = item.to_s | |||
end | |||
elsif /vault book/.match(command) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line 257 uses command == 'vault standard', the two new ones use /vault book/.match(command). I don't know this for sure, but .match does a regular expression comparison, which I suspect is more labor intensive than just doing a string compare. It's probably fairly insignificant, but I would think we're better off with consistency, in any event.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One other thought, you should probably combine the vault book and storage book, since the code itself is the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My initial thought was just being against"read my vault book" and "read my storage book" as the matches and typically just use regex. Can change to whatever.
I was following the example of vault and family vault being unique methods when I made the updates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, not the methods, I just mean these two elsif statements. But just my two cents!
elsif /vault book/.match(command)
# items in vault book output are prefixed by 6 spaces
if line =~ /\s{6}(?:a|an|some) /
item.concat(" (in container - #{container})")
else
container = item.to_s
end
elsif /storage book/.match(command)
# items in a storage book are prefixed by 6 spaces
if line =~ /\s{6}(?:a|an|some) /
item.concat(" (in container - #{container})")
else
container = item.to_s
end
I like the change to retry the command. I've noticed once or twice that if I get stuck in RT, it'll end up putting empty data in until run again. I wish Lich::Util.issue_command had built in support for handling RT. I wonder if it makes sense to create a helper method in common.lic that calls Lich::Util.issue_command similar to DRC.bput. I feel like there is a risk that it could get stuck in a never ending loop (such as when checking the eddy inventory, if you don't have an eddy), but I'm not sure that really needs to be handled. I guess it could do a tap portal before issuing the command. And good catch on the method name for the eddy. No idea how I didn't notice that. For the vault surfaces, it looks like if something is in a container on a surface, the surface will get overwritten. I started looking at this, and I realized there was a bug in the existing code with nested containers. We could do something like this to address the issue: Add below container = "":
They will then save like this:
|
I was going to start looking into this nested container stuff next, but there are too many things that aren't working I wanted to get the initial pull request in to resolve those errors at least. |
I wasn't sure if it was my network or what on the |
combined into single block for the main loop. updated to use double string evaluation vs regex. |
…aracters to prevent overflow into default case.
…aracter inventory.
@mrhoribu worth looking into these timeouts separately. |
…edge case first words.
inventory-manager.lic:
Lich::Util.issue_command
when timing outbase.yaml: