Unshadow remaining core & non-core vars #17138
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ilmotta
requested review from
vkjr,
flexsurfer,
yqrashawn,
Parveshdhull,
J-Son89 and
mohsen-ghafouri
Jenkins BuildsClick to see older builds (27)
|
ilmotta
force-pushed
the
unshadow-vars
branch
2 times, most recently
from
c16f1a8
to
141f9ef
Compare
J-Son89
approved these changes
Aug 29, 2023
Parveshdhull
approved these changes
Aug 29, 2023
ilmotta
force-pushed
the
unshadow-vars
branch
2 times, most recently
from
50a7aa7
to
b2a94d8
Compare
vkjr
approved these changes
Aug 29, 2023
44% of end-end tests have passedFailed tests (24)Click to expandClass TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (19)Click to expandClass TestCommunityOneDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
|
|
@status-im/mobile-qa, I don't want to disrupt your priorities too much. In this PR, only small and controlled changes were made to the code. It's purely refactor in the strict sense of the word. But, I might have introduced a bug, right? Because there's a large number of e2e tests failing, I'm not sure if I can trust them. Could you tell me if you think the failures are related to this PR? Maybe I should just add the Thanks! |
yqrashawn
approved these changes
Aug 30, 2023
The reasons of the failures are known, but I restarted the failed tests just in case. I'll check the results and let you know. |
56% of end-end tests have passedFailed tests (19)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
Passed tests (24)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
|
32% of end-end tests have passedFailed tests (13)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMergedTwo:
Passed tests (6)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityOneDeviceMerged:
|
|
@ilmotta the e2e failures are known, I also smoke-checked the PR, looks good. |
ilmotta
force-pushed
the
unshadow-vars
branch
2 times, most recently
from
2ac7d07
to
c65676d
Compare
ilmotta
force-pushed
the
unshadow-vars
branch
from
c65676d
to
7930806
Compare
alwx
pushed a commit
that referenced
this pull request
Sep 6, 2023
Unshadows all remaining vars in status-mobile, including non cljs.core/clojure.core ones. The only exceptions are cljs.core/type and cljs.core/name (which happen quite often, so I'm not sure if it's worth unshadowing them).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR unshadows all remaining vars in status-mobile, including non
cljs.core/clojure.coreones. The only exceptions arecljs.core/typeandcljs.core/name(which happen quite often, so I'm not sure if it's worth unshadowing them).To avoid rebase hell, please help review this PR in a timely manner 🙏🏼 🙏🏼 .
Out of scope
I refrained from improving code and focused exclusively on satisfying clj-kondo's shadowed-var linter.
Areas that may be impacted
In theory nothing, but I'll make sure to run e2e tests and check the results carefully.
status: ready