Refactor team `roles` to use `role` from response. by abrightwell · Pull Request #6 · CrunchyData/bridge-cli

abrightwell · 2022-01-19T22:32:31Z

These changes refactor the teams command to utilize the role field
found in the teams response as roles is no deprecated.

Example:

> ./bin/cb teams
<personal_id>      personal
<team-1-id>      adam-test-team                  Member, Manager, Administrator
<team-2-id>      <script>alert(1)</script>       Member, Manager, Administrator

> ./bin/cb teams
<personal_id>      personal
<team-1-id>      adam-test-team                  Admin
<team-2-id>      <script>alert(1)</script>       Admin

abrightwell · 2022-01-19T22:34:17Z

src/cb/client.cr

+      if @role.presence
+        @role = (@role == "admin") ? "Administrator" : @role
+        @role.as(String).titleize
+      end


I was hoping this could be a bit simpler and on the surface I felt like it could be. However, since role is String | Nil it seemed like it was necessary (or at least the compiler made it seem that way 🤷). Is there a better way around this?

role is a union of String and Nil because you defined it as a role : String?. The question mark is a shortcut meaning the same thing. Also the return type of this method is String? because if the conditional evaluates to false, the if block returns nil otherwise a string.

You might have thought that the check to @role.presence would mean that inside that conditional the complier would know that @role is not nil. However that assumption isn't thread-safe, since each access to an instance variable could have been changed from another thread. So that's why it's still String? inside the conditional. You could store @role in a local variable and do the conditional on that instead since the local variable wont change.

However, if we know that the api will always return a string, we can drop the ? there and it'd be only of type String. This would also remove the conditional, and change the return type of the method to just String which is nice.

As it stands now though, if the api can actually return a nil/null for that field though, that cast with as will fail: https://play.crystal-lang.org/#/r/cnp9 . The local variable trick would fix that.

I'd also be okay with changing the display of the role it to just the word, admin if that is what the api returns, then we wouldn’t need the overwrite with "Administrator" conditional.

If we do both [be okay with just admin] and [make the type String sans ?], we could move the title casing to when it's displayed in program.cr and actually remove this #role method altogether.

will

So now there is only one role instead of a list of roles?

will · 2022-01-24T14:19:08Z

src/cb/client.cr

+      if @role.presence
+        @role = (@role == "admin") ? "Administrator" : @role
+        @role.as(String).titleize
+      end


role is a union of String and Nil because you defined it as a role : String?. The question mark is a shortcut meaning the same thing. Also the return type of this method is String? because if the conditional evaluates to false, the if block returns nil otherwise a string.

You might have thought that the check to @role.presence would mean that inside that conditional the complier would know that @role is not nil. However that assumption isn't thread-safe, since each access to an instance variable could have been changed from another thread. So that's why it's still String? inside the conditional. You could store @role in a local variable and do the conditional on that instead since the local variable wont change.

However, if we know that the api will always return a string, we can drop the ? there and it'd be only of type String. This would also remove the conditional, and change the return type of the method to just String which is nice.

As it stands now though, if the api can actually return a nil/null for that field though, that cast with as will fail: https://play.crystal-lang.org/#/r/cnp9 . The local variable trick would fix that.

I'd also be okay with changing the display of the role it to just the word, admin if that is what the api returns, then we wouldn’t need the overwrite with "Administrator" conditional.

If we do both [be okay with just admin] and [make the type String sans ?], we could move the title casing to when it's displayed in program.cr and actually remove this #role method altogether.

abrightwell · 2022-01-24T18:43:26Z

However, if we know that the api will always return a string, we can drop the ? there and it'd be only of type String. This would also remove the conditional, and change the return type of the method to just String which is nice.

Yeah, unfortunately, right now null is possible if the team is considered personal.

I'd also be okay with changing the display of the role it to just the word, admin if that is what the api returns, then we wouldn’t need the overwrite with "Administrator" conditional.

WFM. Initially my thoughts were simply around preserving current behaviors. But if you're cool with simply admin then 👍.

I'll update accordingly.

abrightwell · 2022-01-24T18:46:32Z

So now there is only one role instead of a list of roles?

Yes. roles is now deprecated in favor of the singular role.

will · 2022-01-25T14:53:26Z

Yeah, unfortunately, right now null is possible if the team is considered personal.

Do you think the api should return admin for that, since you are the admin of your own personal team?

abrightwell · 2022-01-25T16:19:14Z

Do you think the api should return admin for that, since you are the admin of your own personal team?

That's a good question. @brandur thoughts here?

These changes refactor the `teams` command to utilize the `role` field found in the `teams` response as `roles` is no deprecated.

brandur · 2022-01-25T17:22:52Z

That's a good question. @brandur thoughts here?

No strong preference from me. Yeah, maybe we should change it if it simplifies the implementation of API consumers like it would seem to here.

abrightwell · 2022-01-25T21:19:58Z

src/cb/program.cr

      output << team.name.ljust(name_max).colorize.t_name
      output << "\t"
-      output << team.human_roles.join ", "
+      output << team.role.to_s.titleize


How about this for now? Since nil will be "". That way impact is minimal/negligible regardless of the direction that API takes in the near term.

will · 2022-01-27T16:01:18Z

Thanks, did the rebase on my machine

``` <<< /nix/store/6sk9jhxaqa01jm0wp0b4x9labcyi5a3c-cb-3.6.7.drv >>> /nix/store/jwxavs1iq4zp0bshivkz508js1s8f6g2-cb-3.6.7.drv Version changes: [C.] #1 binutils-with-gold 2.44.tar.bz2 x2 -> 2.44.tar.bz2 [C.] #2 builder.pl <none> x3 -> <none> x2 [C.] #3 cctools-binutils-darwin-wrapper 1010.6 x7 -> 1010.6 x5 [C.] #4 clang 20.1.8, 21.1.2 x2 -> 21.1.2 x2 [C.] #5 clang-at-least 16-LLVMgold-path.patch x4 -> 16-LLVMgold-path.patch x3 [C.] #6 clang-src 20.1.8, 21.1.2 x2 -> 21.1.2 x2 [C.] #7 clang-wrapper 20.1.8 x3, 21.1.2 x3 -> 21.1.2 x3 [C.] #8 compiler-rt 20.1.8, 21.1.2 -> 21.1.2 [C.] #9 compiler-rt-libc 20.1.8, 21.1.2 -> 21.1.2 [C.] #10 compiler-rt-src 20.1.8, 21.1.2 -> 21.1.2 [C.] #11 cpio 2.15 x3, 2.15.tar.bz2 x2 -> 2.15 x2, 2.15.tar.bz2 [C*] #12 crystal 1.10.1-1-darwin-universal.tar.gz, 1.16.3 -> 1.10.1-1-darwin-universal.tar.gz, 1.18.2 [C.] #13 expand-response-params <none> x5 -> <none> x4 [C.] #14 gnu-install-dirs.patch <none> x4 -> <none> x3 [C.] #15 jq 1.8.1 x2, 1.8.1.tar.gz x2 -> 1.8.1, 1.8.1.tar.gz [C.] #16 libbfd-plugin-api-header <none> x3 -> <none> x2 [C.] #17 libcxx 19.1.2+apple-sdk-15.5 x5 -> 19.1.2+apple-sdk-15.5 x4 [C.] #18 llvm 18-compatibility.patch, 20.1.8, 21.1.2 x2 -> 18-compatibility.patch, 21.1.2 x2 [C.] #19 llvm-src 20.1.8, 21.1.2 x2 -> 21.1.2 x2 [C.] #20 llvm-tblgen 20.1.8, 21.1.2 x2 -> 21.1.2 x2 [C.] #21 llvm-tblgen-src 20.1.8, 21.1.2 x2 -> 21.1.2 x2 [C.] #22 macOS-SDK 11.3 x2, 14.4, 15.5 x3 -> 11.3 x2, 14.4, 15.5 x2 [C.] #23 make-binary-wrapper-hook <none> x4 -> <none> x3 [C.] #24 onig 6.9.10.tar.gz x2 -> 6.9.10.tar.gz [C.] #25 oniguruma 6.9.10 x2 -> 6.9.10 [C.] #26 pbzx 1.0.2 x3 -> 1.0.2 x2 [C.] #27 psutil 7.1.0.tar.gz x2 -> 7.1.0.tar.gz [C.] #28 python3 3.13.8 x2, 3.13.8-env x2 -> 3.13.8 x2, 3.13.8-env [C.] #29 python3.13-psutil 7.1.0 x2 -> 7.1.0 [C*] #30 source <none> x85 -> <none> x83 [C*] #31 stdenv-darwin <none> x5 -> <none> x3 Removed packages: [R.] #1 bd49bbaaafc98433a2cb4e95ce25b7a201baf5a5.patch <none> Closure size: 1239 -> 1201 (806 paths added, 844 paths removed, delta -38, disk usage -161.7KiB). ```

* Allow temp location to change if necessary To keep the specs completely isolated, we don't want to reuse the same global tempdir. * update crystal 1.16 -> 1.18 ``` <<< /nix/store/6sk9jhxaqa01jm0wp0b4x9labcyi5a3c-cb-3.6.7.drv >>> /nix/store/jwxavs1iq4zp0bshivkz508js1s8f6g2-cb-3.6.7.drv Version changes: [C.] #1 binutils-with-gold 2.44.tar.bz2 x2 -> 2.44.tar.bz2 [C.] #2 builder.pl <none> x3 -> <none> x2 [C.] #3 cctools-binutils-darwin-wrapper 1010.6 x7 -> 1010.6 x5 [C.] #4 clang 20.1.8, 21.1.2 x2 -> 21.1.2 x2 [C.] #5 clang-at-least 16-LLVMgold-path.patch x4 -> 16-LLVMgold-path.patch x3 [C.] #6 clang-src 20.1.8, 21.1.2 x2 -> 21.1.2 x2 [C.] #7 clang-wrapper 20.1.8 x3, 21.1.2 x3 -> 21.1.2 x3 [C.] #8 compiler-rt 20.1.8, 21.1.2 -> 21.1.2 [C.] #9 compiler-rt-libc 20.1.8, 21.1.2 -> 21.1.2 [C.] #10 compiler-rt-src 20.1.8, 21.1.2 -> 21.1.2 [C.] #11 cpio 2.15 x3, 2.15.tar.bz2 x2 -> 2.15 x2, 2.15.tar.bz2 [C*] #12 crystal 1.10.1-1-darwin-universal.tar.gz, 1.16.3 -> 1.10.1-1-darwin-universal.tar.gz, 1.18.2 [C.] #13 expand-response-params <none> x5 -> <none> x4 [C.] #14 gnu-install-dirs.patch <none> x4 -> <none> x3 [C.] #15 jq 1.8.1 x2, 1.8.1.tar.gz x2 -> 1.8.1, 1.8.1.tar.gz [C.] #16 libbfd-plugin-api-header <none> x3 -> <none> x2 [C.] #17 libcxx 19.1.2+apple-sdk-15.5 x5 -> 19.1.2+apple-sdk-15.5 x4 [C.] #18 llvm 18-compatibility.patch, 20.1.8, 21.1.2 x2 -> 18-compatibility.patch, 21.1.2 x2 [C.] #19 llvm-src 20.1.8, 21.1.2 x2 -> 21.1.2 x2 [C.] #20 llvm-tblgen 20.1.8, 21.1.2 x2 -> 21.1.2 x2 [C.] #21 llvm-tblgen-src 20.1.8, 21.1.2 x2 -> 21.1.2 x2 [C.] #22 macOS-SDK 11.3 x2, 14.4, 15.5 x3 -> 11.3 x2, 14.4, 15.5 x2 [C.] #23 make-binary-wrapper-hook <none> x4 -> <none> x3 [C.] #24 onig 6.9.10.tar.gz x2 -> 6.9.10.tar.gz [C.] #25 oniguruma 6.9.10 x2 -> 6.9.10 [C.] #26 pbzx 1.0.2 x3 -> 1.0.2 x2 [C.] #27 psutil 7.1.0.tar.gz x2 -> 7.1.0.tar.gz [C.] #28 python3 3.13.8 x2, 3.13.8-env x2 -> 3.13.8 x2, 3.13.8-env [C.] #29 python3.13-psutil 7.1.0 x2 -> 7.1.0 [C*] #30 source <none> x85 -> <none> x83 [C*] #31 stdenv-darwin <none> x5 -> <none> x3 Removed packages: [R.] #1 bd49bbaaafc98433a2cb4e95ce25b7a201baf5a5.patch <none> Closure size: 1239 -> 1201 (806 paths added, 844 paths removed, delta -38, disk usage -161.7KiB). ``` --------- Co-authored-by: Will Leinweber <will@bitfission.com>

abrightwell requested a review from will January 19, 2022 22:32

abrightwell commented Jan 19, 2022

View reviewed changes

will requested changes Jan 24, 2022

View reviewed changes

abrightwell force-pushed the abrightwell/br-816 branch from c3a7963 to 2e25ac2 Compare January 25, 2022 16:40

Refactor team roles to use role from response.

e88c46a

These changes refactor the `teams` command to utilize the `role` field found in the `teams` response as `roles` is no deprecated.

abrightwell force-pushed the abrightwell/br-816 branch from 2e25ac2 to e88c46a Compare January 25, 2022 17:04

abrightwell commented Jan 25, 2022

View reviewed changes

abrightwell requested a review from will January 25, 2022 21:20

will closed this Jan 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor team `roles` to use `role` from response.#6

Refactor team `roles` to use `role` from response.#6
abrightwell wants to merge 1 commit intoCrunchyData:mainfrom
abrightwell:abrightwell/br-816

abrightwell commented Jan 19, 2022 •

edited

Loading

Uh oh!

abrightwell Jan 19, 2022

Uh oh!

will Jan 24, 2022

Uh oh!

will left a comment

Uh oh!

will Jan 24, 2022

Uh oh!

abrightwell commented Jan 24, 2022

Uh oh!

abrightwell commented Jan 24, 2022

Uh oh!

will commented Jan 25, 2022

Uh oh!

abrightwell commented Jan 25, 2022

Uh oh!

brandur commented Jan 25, 2022

Uh oh!

abrightwell Jan 25, 2022

Uh oh!

will commented Jan 27, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

abrightwell commented Jan 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

abrightwell Jan 19, 2022

Choose a reason for hiding this comment

Uh oh!

will Jan 24, 2022

Choose a reason for hiding this comment

Uh oh!

will left a comment

Choose a reason for hiding this comment

Uh oh!

will Jan 24, 2022

Choose a reason for hiding this comment

Uh oh!

abrightwell commented Jan 24, 2022

Uh oh!

abrightwell commented Jan 24, 2022

Uh oh!

will commented Jan 25, 2022

Uh oh!

abrightwell commented Jan 25, 2022

Uh oh!

brandur commented Jan 25, 2022

Uh oh!

abrightwell Jan 25, 2022

Choose a reason for hiding this comment

Uh oh!

will commented Jan 27, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

abrightwell commented Jan 19, 2022 •

edited

Loading