Rewriting the user modify command

[JacksonChen666]

Currently a placeholder. Only about the structure of commands (and maybe the parameters) until agreed.

Initial idea: https://github.com/JOJ0/synadm/issues/100#issuecomment-1704264088

Resolves #100

Placeholder/existing commands checklist

(Commits history will 100% get rewritten after the code is finished)

---

Progress checklist:

• [x] Placeholders (see checklist and https://github.com/JOJ0/synadm/pull/126#issuecomment-1733108725)
• [x] Agreed placeholders (agreed)
• [x] Implementation
• [ ] TODOs (deprecating, check synadm user reactivate with --batch)
• [ ] Testing everything new (including deprecation warning)
• [ ] Make README up to date with these changes

[JOJ0]

Thanks for getting this started!

[JacksonChen666]

@JOJ0 Please review the structure of the commands and the API and request any changes.

[JacksonChen666]

checklist of what's been planned/already done so far:

• [x] password
• [x] display name
• [x] avatar URL
• [ ] changing 3pid
• [ ] external_ids (not part of synadm user modify?)
• [x] admin grant
• [x] admin revoke
• [x] deactivation
• [x] reactivation
• [x] locking
• [x] unlocking
• [x] user types

[JacksonChen666]

Conclusive thoughts: If there are verbs in a command they are self-descriptive. Eg. user deactivate

If there are not they are not 100% obvious what they do (display or modify). Eg. user display-name

Not sure if I'm nitpicking here too much or if the priority should be "shorter" names. Eg. set-display-name is rather tedious to type and might be a bad choice...

It should be with a set if it's going to set it. I didn't have plans to make it retrieve the display name and display it. Maybe a --set flag which can set a display name could work, while it could be handy to also see the display name (get by default, which will use the user query API).

[JacksonChen666]

checklist of what's been planned/already done so far: - [ ] changing 3pid - [ ] external_ids (not part of synadm user modify?)

I'm gonna leave those behind for later, keeping the synadm user modify command, maybe implementing modifying those later and deprecating when replacements have been implemented.

[JOJ0]

checklist of what's been planned/already done so far:

• [x] password
• [x] display name
• [x] avatar URL ...

Nice summary! Why not move it to the initial description of this PR? Easier to find and also for future reference what's still missing/postponed (as I just read in your subsequent post you plan on doing)

[JOJ0]

Conclusive thoughts: If there are verbs in a command they are self-descriptive. Eg. user deactivate If there are not they are not 100% obvious what they do (display or modify). Eg. user display-name Not sure if I'm nitpicking here too much or if the priority should be "shorter" names. Eg. set-display-name is rather tedious to type and might be a bad choice...

It should be with a set if it's going to set it. I didn't have plans to make it retrieve the display name and display it. Maybe a --set flag which can set a display name could work, while it could be handy to also see the display name (get by default, which will use the user query API).

Good question what's best....

I just had a look on your work so far in reality/--help:

  admin-grant
  admin-revoke
   ...
  set-display-name     Set the display name of a user.
  set-profile-picture  Set a profile picture for a user by the MXC URI.
  set-user-type
  ...

It seems there is no one-way of doing it right - verb in front, verb on the end of the command. Too long, too short command names....I'm not sure where to go actually....

One thing I'm wondering though: --help as the go-to move for most people (I hope! ;-)) - If we would find a way to be able to manually sort how the options are presented it would be less of a headache for the naming....

let me check, I think I researched this already ages ago and by default Click doesn't have it.....

https://github.com/pallets/click/issues/672

https://github.com/pallets/click/issues/513

[JacksonChen666]

checklist of what's been planned/already done so far: Nice summary! Why not move it to the initial description of this PR? Easier to find and also for future reference what's still missing/postponed (as I just read in your subsequent post you plan on doing)

See my PR description:

Resolves #100

Placeholder/existing commands checklist

Progress checklist:

• [x] Placeholders (see checklist and Rewriting the user modify command #126 (comment))
• [ ] Agreed placeholders
• [ ] Implementation
• [ ] Make README up to date with these changes

[JOJ0]

Ah ok, not sure how you ment that. You will move the checklist up into the description once it's finished?

Anyway, all good, work away! :-)

[JacksonChen666]

Good question what's best....

I think at this point, we have a mostly clear structure although with not so clear command names.

That should allow me to start implementing the commands while we continue deciding on the command names until we find some answer. We can easily change the name later (although less so, the structure, if necessary).

I just had a look on your work so far in reality/--help:

  admin-grant
  admin-revoke
   ...
  set-display-name     Set the display name of a user.
  set-profile-picture  Set a profile picture for a user by the MXC URI.
  set-user-type
  ...

It seems there is no one-way of doing it right - verb in front, verb on the end of the command. Too long, too short command names....I'm not sure where to go actually....

One thing I'm wondering though: --help as the go-to move for most people (I hope! ;-)) - If we would find a way to be able to manually sort how the options are presented it would be less of a headache for the naming....

I have these ideas for command names for setting stuff:

1. set-thingy-thing (current, slightly messes with ordering)
2. thingy-thing set and thingy-thing get (subcommand, not consistent with the rest)
3. thingy-thing --set and thingy-thing --get (command with arg to either set or get)
4. thingy-thing (not as ideal as discussed)
5. set-tt or stt or tt (aliases, not sure if possible, e.g. pfp or set-pfp for set-profile-picture)

[JacksonChen666]

Ah ok, not sure how you ment that. You will move the checklist up into the description once it's finished?

No, not really. The checklist in the PR description shows up in PR listing as how much has been done. Including the placeholders would add to that number, which isn't really desirable since not all commands have been included, which means not 100% of the tasks done.

Although on whether to include checklist or not in the PR description, I'm not sure which side to err on.

[JOJ0]

Please go ahead with implementation and just use what you suggested / we have so far! Yes we could think out for ages on how commands should be named :-) Let's see when implementation is finished whether we'd like to change them.

[JacksonChen666]

Just realized, user modify API can also create users. Not sure how we'll deal with that, we might have to keep the user modify command around and possibly also update it with new stuff.

[JOJ0]

Just realized, user modify API can also create users. Not sure how we'll deal with that, we might have to keep the user modify command around and possibly also update it with new stuff.

Oh, I'm sorry I didn't mention that, I was just assuming you are aware of that. I also didn't realise/think hard enough that it could be a problem with all the "redesign"..... :-/ Sorry my bad!

[JOJ0]

Just realized, user modify API can also create users. Not sure how we'll deal with that, we might have to keep the user modify command around and possibly also update it with new stuff.

Oh, I'm sorry I didn't mention that, I was just assuming you are aware of that. I also didn't realise/think hard enough that it could be a problem with all the "redesign"..... :-/ Sorry my bad!

To wrap this up and partly spice with some new ideas. I was thinking about all this a little:

The current api.user_modify() becomes the user_create() function - we need to keep it anyway as you mentioned. It only is used for create, that should spare us at least a little headache because some things are just not relevant when creating in contrast to modifying). For tri-state options, we could even think if enums could help? Or we keep the current None, True, False business....maybe that's enough. Don't kill me yet, I didn't think through each and every thing we want to set 😅 )

Modifying is not possible anymore with this new method api.user_create()! The newly designed user set... whatever` commands take care of that, each with their own api methods, just as you designed and drafted it already.

Maybe we can reuse _ at least some_ of the cli-options by putting them into the cli.common module? Not sure! Often the wording in --help will be different, which makes reusing not possible.