refactor!: split r/demo/users

[harry-hov]

Splits r/demo/users into p/demo/users and r/demo/users

Related #1393

Contributors' checklist...

• Added new tests, or not needed, or not feasible
• Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
• Updated the official documentation or not needed
• No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
• Added references to related issues and PRs
• Provided any useful hints for running manual tests
• Added new benchmarks to generated graphs, if any. More info here.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 44.60%. Comparing base (75539ea) to head (362b39f).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1433      +/-   ##
==========================================
- Coverage   47.23%   44.60%   -2.64%     
==========================================
  Files         363      460      +97     
  Lines       59493    67872    +8379     
==========================================
+ Hits        28104    30272    +2168     
- Misses      29066    35067    +6001     
- Partials     2323     2533     +210     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[deelawn]

I think this is a great idea. But I also think this can be an opportunity to take this a bit further and separate additional application logic from what we might expect to be a part of a general package any realm can use to manage users.

[deelawn]

Are fields like profile, number, invites, and inviter needed as part of a basic User definition? These seem to be application specific. Or at the very least I should be able to write an application that can couple a user's address with its username without the extra fields I don't care about; that seems to be the base functionality.

But it also might be helpful to provide a struct like UserData or Extended user that provides the extra fields for like applications that want to operate using this data model:

type ExtendedUser struct {
    User
    Profile string
    Number int
    Invites int
    Inviter std.Address
}

[thehowl]

@deelawn agreed, I think it makes sense to keep those fields relevant to r/demo/users (when rendering pages and using its inviting system) so that third party realms/packages only get what they need most of the time (a name associated to an address)

[deelawn]

These two methods also seem very application specific. I don't think there is any obvious reason to prefix usernames with @ in general. Though I do think this type can still be helpful to differentiate a string between being an address or name. This might be a good opportunity to make each method accept a usernamePrefix string argument that allows the application to decide this.

[thehowl]

@deelawn I think the way it currently is makes a lot of sense!

r/demo/users is, to me, the "username" realm every other realm should use to identify users. p/demo/users is not meant to be the "universal" implementation for all user systems.

If we have a realm function which is, say, the following:

func SendMessage(dst users.AddressOrName, title, content string)

having users.AddressOrName as the type indicates that, unless the realm's doing something weird, the function will accept a full address or also a username prefixed with @. So I'm personally in favour to keep it this way; p/demo/users is allowed to have some opinions :)

[deelawn]

A user storage structure seems like a great candidate for something that should be included in a users package. It makes it easy for applications to maintain and access their own user list. Something like:

type p/users.Storage struct {
    nameToUser *avl.Tree
    addresstoUser *avl.Tree
}

func (s *Storage) GetUserByName(name string) *User
func (s *Storage) GetExtendeUserByName(name string) *ExtendedUser
func (s *Storage) getUserByName(name string) interface{}{ return s.nameToUser.Get(name) }

etc
...

[harry-hov]

@deelawn I love your suggestions 💯 However, I would like to stick to refactoring for this PR. I prefer not to introduce any changes to the logic. The main motivation behind this PR is to unblock PR #1393. Once this PR is merged, I will sum up your opinions in a separate issue. WDYT?

[deelawn]

@deelawn I love your suggestions 💯 However, I would like to stick to refactoring for this PR. I prefer not to introduce any changes to the logic. The main motivation behind this PR is to unblock PR #1393. Once this PR is merged, I will sum up your opinions in a separate issue. WDYT?

Yeah fair enough 👍

[thehowl]

I think there's a "guideline" decision to be made here and to place on Effective Gno. Aside from that, the split looks good so pre-approving pending deciding what to do. (Please, in the meantime, do change the method where I commented from String to Render, as it was originally)

[thehowl]

@deelawn I think the way it currently is makes a lot of sense!

r/demo/users is, to me, the "username" realm every other realm should use to identify users. p/demo/users is not meant to be the "universal" implementation for all user systems.

If we have a realm function which is, say, the following:

func SendMessage(dst users.AddressOrName, title, content string)

having users.AddressOrName as the type indicates that, unless the realm's doing something weird, the function will accept a full address or also a username prefixed with @. So I'm personally in favour to keep it this way; p/demo/users is allowed to have some opinions :)

[thehowl]

@deelawn agreed, I think it makes sense to keep those fields relevant to r/demo/users (when rendering pages and using its inviting system) so that third party realms/packages only get what they need most of the time (a name associated to an address)

[jefft0]

The description has the checked item for "No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description" . But the API has a breaking change, removing the getter methods. We will update GnoSocial to use the field without the getter.

func (u User) Name() string         { return u.name }
func (u User) Profile() string      { return u.profile }
func (u User) Address() std.Address { return u.address }

[jaekwon]

Thank you Hariom!