tiled icon indicating copy to clipboard operation
tiled copied to clipboard

Make serve catalog use decorator

Open jwlodek opened this issue 1 year ago • 2 comments

Checklist

  • [X] Add a Changelog entry
  • [X] Add the ticket number which this PR closes to the comment section - N/A

jwlodek avatar Sep 06 '24 15:09 jwlodek

Thanks @jwlodek! I am curious what led you to this change.

I had to grep around to reconstruct my reasoning here. When I added this command we already had:

tiled serve config ...
tiled serve pyobject ...
tiled serve directory ...

and we were adding:

tiled catalog init
tiled catalog upgrade-database
tiled catalog downgrade-database

I couldn't decide which of these two was more obvious:

tiled serve catalog
tiled catalog serve

It seemed inevitable that people would misremember it 50% of the time and feel annoyed, so I registered the same function for both, as you can see in this grep output.

$ git grep serve_catalog
tiled/_tests/test_cli.py:def test_serve_catalog_temp(args, tmp_path):
tiled/commandline/_catalog.py:from ._serve import serve_catalog
tiled/commandline/_catalog.py:catalog_app.command("serve")(serve_catalog)
tiled/commandline/_serve.py:def serve_catalog(
tiled/commandline/_serve.py:serve_app.command("catalog")(serve_catalog)

Should we add a comment to the code explaining this?

danielballan avatar Sep 07 '24 02:09 danielballan

As we discussed a few weeks ago, I'd like to put together a CLI utility for standing up a tiled server for dev work with all the same handlers etc. as beamline tiled, and since I've started prototyping this I had a look at how the tiled cli works currently.

I hadn't seen the same registration in catalog, so I thought this was just something that was missed when moving from explicit registration to decorators for the other functions. I think the reasoning for registering both this way is sound, it makes sense to me. I think a comment explaining the reasoning is a good idea too, so when someone else has a look through the code they don't also assume it was just something that was missed. I'll modify this PR to do so.

jwlodek avatar Sep 07 '24 13:09 jwlodek