Four commands using registerSubstring

I

Irrat

Member
So I was looking at why I can't make a script called "pvpmirror"
Then I realized it's due to some weird command registration behavior.

So now I'm wondering why it's even a thing. Only 4 usages, and only one of them look valid.

Github

https://github.com/kolmafia/kolmafia/blob/main/src/net/sourceforge/kolmafia/KoLmafiaCLI.java#L670
https://github.com/kolmafia/kolmafi...e/kolmafia/textui/command/KitchenCommand.java

Kitchen registers "kitchen", which is likely a mistake? Unless it's supposed to catch a lot of kitchen commands.. "hellskitchen" "dogskitchen" "kitchen" etc.

https://github.com/kolmafia/kolmafia/blob/main/src/net/sourceforge/kolmafia/KoLmafiaCLI.java#L683
https://github.com/kolmafia/kolmafi...kolmafia/textui/command/MirrorLogCommand.java

Mirror registers "mirror" which appears to be so it catches "mirrorstop" "mirrorend" "mirrorclose" as commands

https://github.com/kolmafia/kolmafia/blob/main/src/net/sourceforge/kolmafia/KoLmafiaCLI.java#L727
https://github.com/kolmafia/kolmafi...olmafia/textui/command/RestaurantCommand.java

Not really sure about restaurant which adds "brewery", an alias sure. But why a substring?

https://github.com/kolmafia/kolmafia/blob/main/src/net/sourceforge/kolmafia/KoLmafiaCLI.java#L799
https://github.com/kolmafia/kolmafi.../kolmafia/textui/command/VisitURLCommand.java

The only one that looks valid is the visiturl command. Which is ".php" and should be all good because commands shouldn't have periods in their name anyways. Although, I'd argue that it should be "visiturl page.php" instead. But it's a bit late to suggest that change, and for no real gain.

Changes

So anyways, I want to propose a change for the first three commands.

Kitchen, to register it as "kitchen" and not a substring. This is the one that's most likely to break I think. No idea what people call their kitchens in their scripts.
/hellkitchen is no longer valid, /kitchen

Mirror to be a simple "mirror", with only parameters supporting "end" "stop" "close" as expected. So "mirrorend" will no longer be assigned to any command.
/mirrorend is no longer valid, /mirror end

Restaurant to register "brewery", we could add the rest of the command aliases. But I feel like "Gnomishmicromicrobrewery" is unlikely to be used?
/gnomebrewery is no longer valid, /brewery

Final Notes

Would there be any issues with this, or something I'm missing?

Both "brewery" and "kitchen" I don't think are likely to be in use. And "mirrorend" and the like are more likely to be used in comparison; however I doubt more than a few people are using the "mirror" command. And they should be providing it as a param regardless.
 
Crowther

Crowther

Active member
I use the "mirror" command in a handful of scripts, but never even knew "mirrorend" was a thing.
 
fronobulax

fronobulax

Developer
Staff member
Superfically I think hellkitchen and hellskitchen make sense so if there is a decision to go with this, the script breaking possibility needs to be socialized.

I am not a fan of the mirror change. I have typed mirrorend, for example.

Restaurant looks like

Code: 
    if (cmd.equals("restaurant")) {
      RestaurantCommand.makeChezSnooteeRequest(parameters);
    } else {
      RestaurantCommand.makeMicroBreweryRequest(parameters);
    }

so I would ask how does one access the MicroBrewery without the brewery registration?

At the moment I'm not sure I understand why this is important enough to go forward but I could try harder to understand. If this does advance to a PR four separate PRs might be easier as would possible changes to the commands and not just the registration.
 
I

Irrat

Member
fronobulax said:
Superfically I think hellkitchen and hellskitchen make sense so if there is a decision to go with this, the script breaking possibility needs to be socialized.

I am not a fan of the mirror change. I have typed mirrorend, for example.

Restaurant looks like

Code: 
    if (cmd.equals("restaurant")) {
      RestaurantCommand.makeChezSnooteeRequest(parameters);
    } else {
      RestaurantCommand.makeMicroBreweryRequest(parameters);
    }

so I would ask how does one access the MicroBrewery without the brewery registration?

At the moment I'm not sure I understand why this is important enough to go forward but I could try harder to understand. If this does advance to a PR four separate PRs might be easier as would possible changes to the commands and not just the registration.
Click to expand...

Can add "brewery" as a command alias.

Same for mirror, "mirrorend" and the like added.

The reason I want to make this change is because it's effectively removing "mirror" from a possible script name. Which makes finding a name for the full length mirror semirare script a bit difficult.
 
H

heeheehee

Developer
Staff member
@Veracity I found this old commit:

19626: Fix effect name for "Lucky Cat Is Lucky". Compare decoded names when processin

19626: Fix effect name for "Lucky Cat Is Lucky". Compare decoded names when processin Fix effect name for "Lucky Cat Is Lucky". Compare decoded names when processing effect acquisition. Rationalize almost all "prefix" commands to have two exact match command names rather than any prefix. by...
kolmafia.us

github.com

Fix effect name for "Lucky Cat Is Lucky". Compare decoded names when … · kolmafia/kolmafia@ed34ee4

…processing effect acquisition. Rationalize almost all "prefix" commands to have two exact match command names rather than any prefix. git-svn-id: http://svn.code.sf.net/p/kolmafia/cod...
github.com github.com

To me that supports nixing arbitrary abbreviations:
  • brewery -> brewery / microbrewery
  • mirror -> mirror / mirror{start,begin,open} / mirror{stop,end,close}
  • kitchen -> kitchen / hellskitchen / hellkitchen
I don't know of a good reason to support `let_me_mirror_my_logs_to_this_file mirrored_logs.txt`.

(I've never used a .php command; that should probably also register the prefix https://; I've always used text or visit_url.)
 
fronobulax

fronobulax

Developer
Staff member
On the one hand I see the downward spiral of spending more time discussing this than it would take to just do it and deal with the fallout.

On the other hand I still do not understand the motivation, beyond one person not being able to name a script as they desire. Would we even be having the conversation if the semi-rare was "full length looking glass"?
 
Veracity

Veracity

Developer
Staff member
I’m for nuking command prefixes and just having an assortment of allowed command synonyms.

Well, except for .php, I guess.
 
I

Irrat

Member
heeheehee said:
regarding script names: can you just write "call pvpmirror.ash`?
Click to expand...

> pvpmirror.ash

Mirror stream closed.

fronobulax said:
On the one hand I see the downward spiral of spending more time discussing this than it would take to just do it and deal with the fallout.

On the other hand I still do not understand the motivation, beyond one person not being able to name a script as they desire. Would we even be having the conversation if the semi-rare was "full length looking glass"?
Click to expand...

It's not so much that it's just the mirror, but that it's something that could be fixed. But yeah. I'll whip up some PRs

Also found the commit for mirror streams. It doesn't really explain why it's needed

The first commit
github.com

adding mirror command · kolmafia/kolmafia@a800f8e

KoLmafia is a cross-platform desktop tool which interfaces with the online adventure game Kingdom of Loathing 🗡️🍸 - adding mirror command · kolmafia/kolmafia@a800f8e
github.com github.com

One that adds more on

github.com

improving handling of mirror streams · kolmafia/kolmafia@3db20b7

introducing NULL_STATE for GUI-driven printouts
github.com github.com

I think I missed a commit but it's annoying looking through them.
Don't see a compelling reason regardless.
 
You must log in or register to reply here.
Top