🎄 🧹 New Year cleanup #21868
🎄 🧹 New Year cleanup #21868
Conversation
Jenkins BuildsClick to see older builds (28)
|
vkjr
requested review from
ilmotta,
shivekkhurana,
flexsurfer,
briansztamfater,
smohamedjavid,
clauxx,
mohsen-ghafouri,
ulisesmac and
a team
|
Thanks @vkjr for addressing this cleanup!
@status-im/mobile, is there any reason to keep the Most of the legacy code is only used or referenced in legacy namespaces, so I feel the cleanup could be more extensive this way. If this change is too broad for this PR, we could address it in a separate one. What do you think?
If I remember correctly, we already discussed this during the last offsite and agreed to remove the deprecation. Therefore, removing the deprecation is the right approach, but the guidelines should also be updated accordingly. I'll tag @ilmotta here so we can confirm the outcome of that discussion in Montenegro. |
|
@briansztamfater, I also think that 'legacy' folder should be cleaned more or removed altogether but that would take slightly different approach. Code should be moved around. |
There was a problem hiding this comment.
This is an epic cleanup, well done 🙌
I left some small comments, apologies for any duplicates, it was easier to copy-and-paste some comments about naming.
Overall, I think things look good, though I wonder whether we should avoid suppressing warnings if we plan to do more PRs for resolving the warnings. For example, maybe we want to use the warnings for helping us find which code we want to revise next (?). Let me know your thoughts when you have a chance please 🙏
src/legacy/status_im/ens/core.cljs
Outdated
| st (state custom-domain? username usernames)] | ||
| (reset! resolve-last-id (random/id)) | ||
| (merge | ||
| {:db (update db | ||
| :ens/registration assoc | ||
| :username username | ||
| :state state)} | ||
| (when (= state :searching) | ||
| :state st)} | ||
| (when (= st :searching) |
There was a problem hiding this comment.
Maybe st could be named next-state?
| [{:keys [db] :as cofx} {h :hash :as transaction}] | ||
| (when-let [watch-params | ||
| (get-in db [:ethereum/watched-transactions hash])] | ||
| (get-in db [:ethereum/watched-transactions h])] | ||
| (let [{:keys [trigger-fn on-trigger]} watch-params] | ||
| (when (trigger-fn db transaction) | ||
| (rf/merge cofx | ||
| {:db (update db | ||
| :ethereum/watched-transactions | ||
| dissoc | ||
| hash)} | ||
| h)} |
There was a problem hiding this comment.
Maybe h could be named transaction-hash or tx-hash?
| [{:keys [db] :as cofx} {h :hash :keys [id address] :as transfer}] | ||
| (let [transfer-by-hash (get-in db [:wallet-legacy :accounts address :transactions h])] | ||
| (when-let [unique-id (when-not (= transfer transfer-by-hash) | ||
| (if (and transfer-by-hash | ||
| (not (= :pending | ||
| (:type transfer-by-hash)))) | ||
| id | ||
| hash))] | ||
| h))] |
There was a problem hiding this comment.
Maybe h could be named transaction-hash or tx-hash?
| {h :hash block :block}] | ||
| (cond | ||
| (or (nil? min-block) (> min-block (js/parseInt block))) | ||
| {:min-block (js/parseInt block) | ||
| :min-block-transfers-count 1} | ||
|
|
||
| (and (= min-block block) | ||
| (nil? (get-in db [:wallet-legacy :accounts checksum :transactions hash]))) | ||
| (nil? (get-in db [:wallet-legacy :accounts checksum :transactions h]))) | ||
| (update acc :min-block-transfers-count inc) |
There was a problem hiding this comment.
Maybe h could be named transaction-hash or tx-hash?
| (get-in db [:wallet-legacy :accounts (eip55/address->checksum address) :max-block])) | ||
|
|
||
| (defn min-block-transfers-count | ||
| (defn min-block-transfers-count-fn |
There was a problem hiding this comment.
Maybe this could be named count-min-block-transfers ?
| [{:keys [db]} k value] | ||
| {:db (-> db | ||
| (assoc-in [:bug-report/details key] value) | ||
| (assoc-in [:bug-report/details k] value) | ||
| (dissoc :bug-report/description-error))}) |
There was a problem hiding this comment.
Maybe k could be named log-key?
src/native_module/core.cljs
Outdated
| (defn hash-transaction | ||
| "used for keycard" | ||
| [rpcParams callback] | ||
| (log/debug "[native-module] hash-transaction") | ||
| (.hashTransaction ^js (encryption) rpcParams callback)) | ||
|
|
||
| (defn hash-message | ||
| "used for keycard" | ||
| [message callback] | ||
| (log/debug "[native-module] hash-message") | ||
| (.hashMessage ^js (encryption) message callback)) | ||
|
|
There was a problem hiding this comment.
I wonder if these native bindings should get some extra review since they could still be valuable references to old/upcoming features. Some of these seem they might be related to some upcoming keycard features for signing transactions and etc, but I'm not sure.
There was a problem hiding this comment.
Good point @seanstrom This namespace I'd consider leaving untouched with unused public vars so that it serves as a reference API. Also, if we remove from the Clojure layer, we should remove from the native layer as well in the repo. Maybe some of these functions are not even used by the desktop client, in which case we can also remove from status-go.
There was a problem hiding this comment.
I agree, we should leave it as is, since some of these (hash-typed-data/hash-typed-data-v4) are being used in a WIP keycard branch
| ;; deprecated warning suppressed but please rewrite if you have time | ||
| #_{:clj-kondo/ignore [:deprecated-var]} |
There was a problem hiding this comment.
I wonder if we should try formatting these comments with a prefix like: TODO: deprecated ..., so that they're easier to search for throughout the code. thoughts?
There was a problem hiding this comment.
We have 86 TODOs in the code already) I don't think increasing this value by 100 with those deprecated suppression would make any change. Plus, we aren't really need rewriting what is deprecated. It would be nice, it would be better but it is not really a TODO.
Overall, I think things look good, though I wonder whether we should avoid suppressing warnings if we plan to do more PRs for resolving the warnings.
Overall I'd prefer maintaining a zero-warnings policy (and ensuring it with the automatic PR check). To be sure that if anything triggered a warning in the future - that was seen and fixed. And warnings that we already have... well it is possible that we will fix them but it is also possible that not. In any case old warnings shouldn't stay in our way of preventing new ones. wdyt?
|
@seanstrom, thanks for all the comments, agree with the renamings! |
There was a problem hiding this comment.
Such a nice gift for Christmas @vkjr! Thank you!
react-native.core/use-effect was removed from deprecateds.
We had an agreement between us that use-effect shouldn't be considered deprecated, so I think this is fine.
"deprecated" flag removed from re-frame/defn declaration.
Makes sense to me. We all know this is super deprecated and every now and then people rewrite usages of this macro.
The sticky point for me is that I don't currently agree with the addition of so many warning suppression comments.
When we run make lint we ignore all clj-kondo warnings via grep, thus why do we want to suppress warnings twice (code & make target)? When the dev wants to see the full list of warnings, they can run make lint CLJ_LINTER_PRINT_WARNINGS=true. The dev can also easily pipe a grep to filter/focus on certain warnings.
This is a flexible approach that doesn't require adding suppression comments.
When we add suppression comments, we are removing a nice hint in text editors for the developer to see the warning and we are also making the code noisy because the code #_{:clj-kondo/ignore [:deprecated-namespace]} is noisier than a squiggle line or hint (depending on the text editor). The suppression comment is also tied to the code structure and must be moved around correctly.
When I work with warnings, I don't care about the whole list of warnings because I want to fix one type of warning at a time, and for that reason I grep only specific warnings.
The correct IMO is to resolve warnings in bite-size PRs, not suppress them (unless it's really an exception).
| @@ -548,8 +502,8 @@ | |||
|
|
|||
| (rf/defn lock-pressed | |||
| {:events [:browser.ui/lock-pressed]} | |||
| [cofx secure?] | |||
| (update-browser-option cofx :show-tooltip (if secure? :secure :not-secure))) | |||
| [cofx is-secure?] | |||
There was a problem hiding this comment.
Why this change from secure? to is-secure? ? secure? follows the Clojure style guide our style guide.
There was a problem hiding this comment.
probably was shadowing something, will check!
| @@ -1,8 +1,7 @@ | |||
| (ns legacy.status-im.utils.utils-test | |||
| (:require | |||
| [cljs.test :refer-macros [deftest is]] | |||
| [legacy.status-im.utils.core :as u] | |||
| [legacy.status-im.utils.utils :as uu])) | |||
| [legacy.status-im.utils.core :as u])) | |||
There was a problem hiding this comment.
We sort of have a convention to use sut as the alias for the namespace under test. Could you alias to sut instead of u?
src/native_module/core.cljs
Outdated
| (defn hash-transaction | ||
| "used for keycard" | ||
| [rpcParams callback] | ||
| (log/debug "[native-module] hash-transaction") | ||
| (.hashTransaction ^js (encryption) rpcParams callback)) | ||
|
|
||
| (defn hash-message | ||
| "used for keycard" | ||
| [message callback] | ||
| (log/debug "[native-module] hash-message") | ||
| (.hashMessage ^js (encryption) message callback)) | ||
|
|
There was a problem hiding this comment.
Good point @seanstrom This namespace I'd consider leaving untouched with unused public vars so that it serves as a reference API. Also, if we remove from the Clojure layer, we should remove from the native layer as well in the repo. Maybe some of these functions are not even used by the desktop client, in which case we can also remove from status-go.
src/test_helpers/unit.cljs
Outdated
| :test/assoc-in | ||
| (fn [app-db [_ path value]] | ||
| (assoc-in app-db path value)))) | ||
|
|
||
| (defn spy-event-fx |
There was a problem hiding this comment.
When I wrote abstractions like spy-event-fx I was trying to come up with an intermediary solution between our current integration tests and unit testing event handlers like we do now. I didn't like the experience and the brittleness of this spying/stubbing solution for events, so I stopped using this.
If you can remove in this PR, please remove spy-event-fx, spy-fx, stub-fx-with-callbacks. I think db function can also be removed if there are no usages.
| @@ -39,15 +39,23 @@ | |||
| (defn percentage-change | |||
| [customization-color theme] | |||
| {:color (colors/theme-colors | |||
| ;; deprecated warning suppressed but please rewrite if you have time | |||
There was a problem hiding this comment.
Feels excessive to have two comments the same thing. The clj-kondo clearly means it's suppressing a warning. If you agree, could you remove all such ;; deprecated warning ... comments?
There was a problem hiding this comment.
I won't insist too much on having also textual comment but think that it makes better call to action in comparison to just having a suppressed warning. So better chance to have warnings fixed :)
There was a problem hiding this comment.
@vkjr I think we just need to prioritize some tech debt work every now and then since it's not happening organically. In particular, warning removal tends to go low in most devs' priorities, so it's natural that they accumulate over time. This happened in all projects I worked in. Adding more comments to increase noise may work, but I don't hold my hopes high.
.clj-kondo/config.edn
Outdated
| ;; be removed: | ||
| keycard.keycard | ||
| test-helpers.unit | ||
| test-helpers.component |
There was a problem hiding this comment.
The unused vars in test-helpers.component: after-each, test-only, describe-only, and describe-skip can also be used only temporarily during development, thus not necessarily to commit. For this namespace in particular I think we can just ignore the unused public vars, but shouldn't consider deleting.
Yes, currently we ignore warnings in linting. And we allow the new code to be committed with warnings. But I suggest to only commit code without warnings :) To start forcing "no-warnings" policy we need to get rid of ALL existing warnings. Lets be honest, we won't make anytime soon an effort to really remove them. But that shouldn't stop us from implementing no-warnings policy. We can suppress an existing ones and never add new. And slowly fix already existing but suppressed.
Yes, editor hint is nice, but as we can see from the experience, having a thousand hints in editor just makes us blind to the warnings :) On the other side when there were no warnings in And I don't really think that ugliness of suppression comment is a bad thing. Maybe it is something that would push us to fix those parts instead of ignoring them. So in the end of the day my suggestion is to have all existing warnings suppressed and start having no-warnings policy. What do you think? I'd be happy to hear more opinions on this point from the team. |
|
@ilmotta, thanks for checking the pr! 🙏 |
|
@vkjr I like the idea of avoiding deprecation warnings and limiting what kind of code we check-in to the codebase 👍 Though I think we should try to resolve the current warnings as soon as possible to get the best benefits. So with that in mind I've tried to help with cleaning up some of the new deprecation warning suppressions (the one's introduced in this PR) by creating another PR based on these changes that try to resolve a good chunk of the warnings there: #21874 Hopefully this will help with merging this PR and enabling the zero-warnings policy 🙌 Lmk your thoughts when you have a chance 🙏 |
|
@seanstrom, wow, thanks a lot! |
There was a problem hiding this comment.
😮 Thanks a lot @vkjr!!
IMO it would make sense to start with a zero-warnings policy only after we actually solve all the warnings (like you and @seanstrom did), instead of leaving some suppressed. Otherwise suppressing warnings might become a path of least resistance for CCs if we have examples of that in the codebase.
|
@ilmotta, @clauxx, @seanstrom, @briansztamfater |
@vkjr, what you are suggesting is exactly how clj-kondo was configured in the past. Warnings were considered errors and We changed this configuration because we wanted to leverage the @seanstrom @clauxx @vkjr Going back to a zero-warning policy also means going back to where we were before, where we sometimes wouldn't make the first move because it would be too expensive for any single developer to attack the problem. I remember when we started to allow cheap deprecation via metadata (not renaming vars which is a world of pain) we started to effectively deprecate code and gradually evolve instead of being stuck. There are also some linters which can generate false positive and these can be useful too (like our translation linter). I can live with either approach guys, but prefer the flexibility of allowing warnings. If the majority prefers, the best to reinforce the policy is to change clj-kondo I'll try to fix some of the warnings to help. I see @seanstrom already fixed some in another PR.
Maybe it's just me, but I 95% of the time care about the warnings in the files I'm working on. I don't fully understand why checking all the possible warnings in the entire project is useful during development. My Emacs only reports warnings for the buffers I'm in and usually there are very few (if any) warnings per buffer. In which way does your Emacs throw thousands of warnings to you? Could you detail a little bit how the warnings are affecting your workflow?
Suppression comments are okay if there aren't too many, otherwise I think it's better for the teams to prioritize some work to attack the tech debt instead of indirectly using comments. It's similar to TODO comments, they basically don't work unless there are very few and that's why we added to the guidelines to not use them. It's like the broken windows theory, if we have too many suppression comments/TODO comments nobody will care. But hey, this PR is bringing the problem back to the spotlight, which is a great move! Probably a better solution to generate noise about warnings is to make them visible during continuous integration, similar to how status-go reports test coverage in PRs. This way PR author and reviewers would easily spot when PRs are making things worse or when the number of warnings exceeded a certain threshold. |
84% of end-end tests have passedFailed tests (6)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Class TestDeepLinksOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Expected to fail tests (3)Click to expandClass TestWalletMultipleDevice:
Class TestFallbackMultipleDevice:
Passed tests (47)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestWalletOneDevice:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestFallbackMultipleDevice:
Class TestDeepLinksOneDevice:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
|
|
Hi @vkjr ! Thanks for your cleanup! PR can be merged. |
|
@mariia-skrypnyk, thank you! |
|
@vkjr a bit late, but thanks for this PR! :) 💯 |
Summary
We had 1000 warnings reported by clj-kondo.
Problem with having tons of warnings is that we are missing all the helpful tips, we just cant see them.
So what was done to deal with warnings:
Though few exceptions made, you can find them in
.clj-kondo/config.ednin{:linters {:clojure-lsp/unused-public-var {:exclude ..}}section.re-frame/defndoesn't throwunused public varwarning anymore, that was fixed in.clj-kondo/config.ednre-frame/defndeclaration. Because there are around 500 usages of it and this warning occupies all the log.react-native.core/use-effectwas removed from deprecateds. Its usage generates a lot of warnings, but explanation why it shouldn't be used in guidelines is not quite clear. So if we really want to get rid ofuse-effect, lets make it a separate tech debt issue and discuss how it should be done.Errors and warnings In develop:
Errors and warnings in this branch:
Review notes
Testing notes
Platforms
Areas that maybe impacted
Potentially everything
Functional
status: ready