Added destructuring section to new-guidelines.md #18731
Conversation
Jenkins BuildsClick to see older builds (32)
|
J-Son89
requested review from
ilmotta,
briansztamfater,
clauxx,
smohamedjavid,
ulisesmac and
OmarBasem
doc/new-guidelines.md
Outdated
| Too often callers pass nil values because values can be wrapped in a `when` for example. | ||
| In this case, the default value is not applied, because :or macro will use default only when the value is absent. | ||
| Instead, use `(or value some-default-value)` in a `let` expression or as a parameter value. | ||
|
|
||
| ```clojure | ||
| ;; bad (unreliable) | ||
| (defn- view-internal | ||
| [{:keys [auto-focus? | ||
| init-value | ||
| return-key-type] | ||
| :or {auto-focus? false | ||
| init-value 0 | ||
| return-key-type :done}}]) | ||
|
|
||
| ;; good | ||
| (defn- view-internal | ||
| [{:keys [auto-focus? | ||
| init-value | ||
| return-key-type]}]) | ||
| (let [auto-focus? (or auto-focus? false) | ||
| init-value (or init-value 0) | ||
| return-key-type (or return-key-type :done)]) |
There was a problem hiding this comment.
I don't fully agree with this approach.
I understand the problems it's creating, but it doesn't look clean, we are also suggesting to shadow vars.
I think this technique should be followed only sometimes, when needed, but these guidelines are thought to be applied everywhere and the code will become (even) more verbose.
to avoid the shadowing, I see cleaner:
(defn view [{default-auto-focus? :auto-focus?
default-init-value :init-value}]
(let [auto-focus? (or default-auto-focus? 20)
init-value? (or default-init-value 0)]))
There was a problem hiding this comment.
Nice suggestion @ulisesmac. I prefer that way too without shadowing.
I think this technique should be followed only sometimes, when needed, but these guidelines are thought to be applied everywhere and the code will become (even) more verbose.
I agree with you. I think we should adjust the wording in the guidelines. Not all guidelines should be followed 100% of the time. This one I think should be considered by all developers. I personally try to never rely on default destructuring in quo components. Too often I've seen arguments been passed as nil. And I believe a good proportion of the dev team doesn't know nil will overrule default destructured values.
Even with Malli it doesn't help much because we're defining schemas for components with heavy usage of the :maybe schema, so nils can still flow in.
Searching by usages of :or, I see many have fragile code, waiting to cause small UI bugs or even exceptions, like here (if :size is passed as nil an exception will be thrown)
status-mobile/src/quo/components/tags/permission_tag.cljs
Lines 137 to 151 in 07feff6
Maybe this guideline should be considered mostly for quo components? So it could be moved to quo/README.md.
There was a problem hiding this comment.
Maybe this guideline should be considered mostly for quo components?
Yeah, that and fix the wording will make it better :)
There was a problem hiding this comment.
I've moved the guideline to the quo/Readme.md, please, take a look.
Also, I don't prefer the naming with the default- prefix, as the actual default will be the second prop to or fn, ex. 20 in (or default-auto-focus? 20). For me, the better naming can be smthng like param-auto-focus? or external-auto-focus?. Probably, shorter is better.
src/quo/README.md
Outdated
| [{:keys [{default-auto-focus? :auto-focus? | ||
| default-init-value :init-value | ||
| default-return-key-type :return-key-type}]}]) |
There was a problem hiding this comment.
This might be wrong actually. It should be without :keys, so just:
(defn- view-internal
[{default-auto-focus? :auto-focus?
default-init-value :init-value
default-return-key-type :return-key-type}]}])We'd have to do this for all props, even ones that have no default values, so using this might be too verbose. Is avoiding shadowing them worth it @ulisesmac @ilmotta?
There was a problem hiding this comment.
Maybe it's better to not use destructuring for props that have default values e.g.
(defn- view-internal
[{:keys [theme size something] :as props}]
(let [auto-focus? (or (:auto-focus? props) false)]
...)}])There was a problem hiding this comment.
Hey @clauxx
We'd have to do this for all props, even ones that have no default values
We can mix destructuring sintax, so we can use {a :a} and {:keys [x]} together., but honestly, I prefer to have only one style.
I like your idea @clauxx, destructuring doesn't look dirty, we avoid shadowing vars and no need to names prefixed with default- 👍
And yeah, the guideline is wrong about the use of :keys
There was a problem hiding this comment.
Shadowing arguments requiring default values in a top-level let form is harmless and quite common in Clojure code I have seen. But I can totally live with the final suggestion to avoid destructuring when there are default values, that's good too, maybe better 👍🏼
There was a problem hiding this comment.
@ilmotta, @ulisesmac, @clauxx, I've updated the PR according to the discussion, please, review it
There was a problem hiding this comment.
LGTM with the latest changes based on our discussions. Thanks @nazariifenii!
@ilmotta, could you please take care of merging the PR, because it seems like I don't have access to do that. |
Sure @nazariifenii. I'm sorry you have to do this again, but could you rebase against |
|
@nazariifenii, the build failed for reasons unrelated to your PR. Other PRs are having the same problem. For now, I think the best is to wait and we can try to merge it next Monday. |
|
Hey @nazariifenii, the pipeline seems to be stable enough. Could you rebase again, please? Thanks! |
|
Done, @ilmotta, thanks for monitoring! |
Summary
Here's a quick PR to add a destructuring section to the new guidelines.
To review the rendered file, please, go to https://github.com/status-im/status-mobile/blob/251fccf7b6d3e00f7a5dc93ea70bbc76dc5433f1/doc/new-guidelines.md#default-value-when-destructuring.
Review notes
The guidelines should contain a section explaining the use cases of destructuring function parameters to improve code style.
status: ready