Feature - Implemented Is Familiar Equipment Locked?

[Bale]

I know that KoLmafia tracks the locking of familiar equipment since it appears on the Gear Changer tab, however this information is not made apparent to the user.

I would like to add locking and unlocking of familiar equipment to Chit. There's already a nice familiar control panel in that relay script for choosing the familiar and his equipment. You can even activate the Moveable Feast from the charpane. It seems like an obvious next step to add a button for locking and unlocking of familiar equipment. However I can't tell if the gear is locked. To do this effectively I need a new function or preference to detect the current state of locking:

boolean familiar_equipment_locked()
returns true if it is locked or false if it is unlocked.

I can always lock or unlock the equipment with visit_url("familiar.php?action=lockequip&pwd="+my_hash()) (relay scripts need my_hash) so I don't need anything else, but I wouldn't mind if someone also wanted to add a function for locking/unlocking equipment.

 

[Catch-22]

I've got familiar_equipment_locked() working in my build (patch attached), but I'm wondering if something like the following would be alright for a locking/unlocking function:

boolean familiar_equipment_locked()
Returns true or false depending if it's locked.

void familiar_equipment_locked(boolean)
Locks/unlocks the equipment for you (if it's not already).

 

[Catch-22]

If overloading the function is acceptable for item locking, this patch adds just that.

 

[Bale]

Has anyone had a chance to look at Catch-22's patch? I'm happy with just the first one and it is really, really short so it shouldn't take very long. His second post is extra credit.

 

[roippi]

My only hangup is that the overloaded signatures seem weird. If we're following the java idiom, boolean is_equipment_locked() and void lock_equipment() seem more natural. I don't know, I'm not much of an ASH guy.

Functionally the patch passes inspection, though I haven't tested it.

 

[Bale]

My only hangup is that the overloaded signatures seem weird. If we're following the java idiom, boolean is_equipment_locked() and void lock_equipment() seem more natural. I don't know, I'm not much of an ASH guy.

Agreed. That would be better. It should be rewritten that way.

 

[Catch-22]

Well, as far as "is_" is concerned, I noticed existing ash functions like white_citadel_available, guild_store_available are lacking an "is_" but then there are functions like "is_accessible", so I went with simply prefixing with "familiar_" (as per Bale's suggestion) as it seems like most familiar related functions in ASH use that prefix.

As for the 2nd function, I figured you could use something like familiar_lock_equipment() but decided not to. Changing the function name is as simple as editing the patch in a text editor before you apply it, though.

I won't be offended if it gets changed, so I guess it's just up to whoever decides to commit

Edit: Didn't notice Bale's comment. My only issue with Roippi's suggested function names is that they don't indicate that it's talking about familiar equipment (function purpose could seem obscure to a novice).

 

[roippi]

Yeah, it's not necessarily that you did anything wrong. There are, to my knowledge, no official standards for ASH, it's just been made up as we go along - and then old functions have not been removed as to maintain backwards-compatibility. If we had some ability to mark deprecation that would be a thing, but that's not a thing as of this writing. Maybe we could make deprecation a thing, I don't know. Maybe we could make some standards, I don't know.

If I were to personally make any recommendations, it would be to follow Java idioms. But as you say, whomever commits this can have the final word on that one. (until the next committer has their final word)

 

[Catch-22]

On the topic of deprecation, it could be done by changing the function do do something like:

function old_function(vars) {
    print("old_function is deprecated, use new_function instead.");
    return new_function(vars);
}

 

[Bale]

We've got a patch and have had a little discussion about how to properly name it. It has languished a little while since then. Roippi, what do you think of commiting this?

 

[roippi]

I'll have a look after I finish the widget that I'm working on.

 

[Bale]

Thank you. I'd really like to use boolean is_equipment_locked() and void lock_equipment().

 

[Catch-22]

Even if we're not overloading the function, I'd still recommend boolean familiar_equipment_locked() and void familiar_lock_equipment() so the function names are self-documenting. I don't have commit powers though, so it's not up to me

 

[Bale]

The problem is that those names sound ambiguously interchangeable. For instance, familiar_equipment_locked could be asking if familiar equipment is locked, or be commanding that familiar equipment be locked. They are just the same three words in a different order with one having a different tense. I believe I know which one is which but they are too potentially confusing. The names as roippi suggested are unambiguous, and more distinctive.

 

[Catch-22]

The ambiguous interchangeability is why I suggested overloading the function in the first place

I don't like it but; is_familiar_equipment_locked() and lock_familiar_equipment() would be less ambiguous and more distinctive.

 

[roippi]

r11197

 

[Bale]

Oh, sweetness! I can't wait for the build so that I start work on adding this into ChIT!

Thanks, roippi.