Add publish_own_name, publish_own_description, get_description #6

notplants · 2021-11-15T11:29:05Z

notplants commented

2021-11-15 11:29:05 +00:00

This PR adds three methods to sbotcli:

publish_own_name (calls publish_name passing the sbots own id as the id)
publish_own_descrption (calls publish_description passing the sbots own id as the id)
get_description (same implementation as get_name)

This PR adds three methods to sbotcli: - publish_own_name (calls publish_name passing the sbots own id as the id) - publish_own_descrption (calls publish_description passing the sbots own id as the id) - get_description (same implementation as get_name)

notplants added 1 commit 2021-11-15 11:29:06 +00:00

c8b3ecc278 Add publish_own_name and publish_own_description

notplants added 1 commit 2021-11-15 11:29:52 +00:00

5837271675 Change ssb_id to id to keep style uniformity

notplants requested review from glyph 2021-11-15 11:30:27 +00:00

glyph requested changes 2021-11-15 12:32:03 +00:00

glyph left a comment

Ace, I love these new self-oriented methods! I've left two comments for you.

src/lib.rs Outdated

						
				@ -348,3 +377,3 @@

				    ///

				    /// * `name` - A string slice representing a profile name (bio)

				    /// * `id` - A string slice representing an id (profile reference / public key)

				    /// * `id` - A string slice representing a id (profile reference / public key) of who the name is for

glyph commented

2021-11-15 12:01:25 +00:00

Hmm this is a bit clumsy. Might be better as:

/// * `id` - A string slice representing the id (profile reference / public key) of the profile being named

Or:

/// * `id` - A string slice representing the id (profile reference / public key) of the one being named

Hmm this is a bit clumsy. Might be better as: ```/// * `id` - A string slice representing the id (profile reference / public key) of the profile being named``` Or: ```/// * `id` - A string slice representing the id (profile reference / public key) of the one being named```

notplants marked this conversation as resolved

src/lib.rs

						
				@ -452,0 +518,4 @@

				                },

				                // if the regex does not match, then return an error

				                None => {

				                    Err(SbotCliError::WhoAmI("Error calling whoami: regex not matched".to_string()))

glyph commented

2021-11-15 12:29:50 +00:00

I'm in two minds about returning an error for the no-capture condition. It's technically not an error, since the whoami call was executed successfully and regex_finder returned Ok. This is why I originally chose to return a Result<Option<String>, SbotCliError>; a None value represents a failure to obtain the id and the caller must match accordingly.

With that being said, I think it's extremely unlikely that we fail to capture the id. That would only happen if our regex pattern were incorrect or if there was a low-level error in gosbot for the whoami method.

If we stick with the current error approach, I'd recommend making the message more specific:

"Error calling whoami: failed to capture the id value using regex"

I'm in two minds about returning an error for the no-capture condition. It's technically not an error, since the `whoami` call was executed successfully and `regex_finder` returned `Ok`. This is why I originally chose to return a `Result<Option<String>, SbotCliError>`; a `None` value represents a failure to obtain the `id` and the caller must match accordingly. With that being said, I think it's extremely unlikely that we fail to capture the `id`. That would only happen if our regex pattern were incorrect or if there was a low-level error in `gosbot` for the `whoami` method. If we stick with the current error approach, I'd recommend making the message more specific: "Error calling whoami: failed to capture the id value using regex"

notplants commented

2021-11-15 13:07:45 +00:00

imo an error is technically not just when an underlying call returns an error, but whenever a function fails to do what you want it to do, however you define that.

To me it makes the most sense to define that if this function is working it returns an id, and if it doesn't then it returns an error, because I can't think of a case where sbot is working but you wouldn't have an id from whoami, and defining it in this way makes it easier for the caller to use in most use cases where the ID is really what they want.

Returning an Option feels to me like creating unnessesary work. Or is there a case where whoami should return None?

imo an error is technically not just when an underlying call returns an error, but whenever a function fails to do what you want it to do, however you define that. To me it makes the most sense to define that if this function is working it returns an id, and if it doesn't then it returns an error, because I can't think of a case where sbot is working but you wouldn't have an id from whoami, and defining it in this way makes it easier for the caller to use in most use cases where the ID is really what they want. Returning an Option feels to me like creating unnessesary work. Or is there a case where whoami should return None?

glyph commented

2021-11-15 13:20:49 +00:00

Agreed that the Option will create unnecessary work for the caller and may result in confusion. Also agreed that the ID value should always be returned from the sbot if the whoami call is successful. Error approach is fine by me 👍

Clear to merge 🟢

Agreed that the `Option` will create unnecessary work for the caller and may result in confusion. Also agreed that the ID value should always be returned from the sbot if the `whoami` call is successful. Error approach is fine by me 👍 Clear to merge 🟢

glyph marked this conversation as resolved

notplants added 1 commit 2021-11-15 13:16:18 +00:00

c300ffd130 Response to code review

notplants merged commit c55b13d026 into main

2021-11-15 13:31:55 +00:00

notplants deleted branch publish_own_name

2021-11-15 13:31:55 +00:00

notplants referenced this issue from a commit

2021-11-15 13:31:55 +00:00

Merge pull request 'Add publish_own_name, publish_own_description, get_description' (#6) from publish_own_name into main

Sign in to join this conversation.

No reviewers