Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Format code with latest code style #3975

Merged
merged 16 commits into from
Mar 30, 2024

Conversation

Bambooin
Copy link
Contributor

@Bambooin Bambooin commented Oct 31, 2023

Description

Format code with latest format tool.

The v2 patch doesn't change any code logic.

I disable some ktlint standard rules in kotlin files:

[*.{kt,kts}]
ktlint_standard_no-wildcard-imports = disabled
ktlint_standard_property-naming = disabled
ktlint_standard_value-argument-comment = disabled
ktlint_standard_kdoc-wrapping = disabled
ktlint_standard_no-consecutive-comments = disabled

For these rules, we can fix them in the future pull request one by one.

Unit test is passed and was verified and tested with LineageOS 20.

The left Codacy issues were almost function too long, and this is can be fixed by update setting.

The last commit fix can be cherry pick to release/3.10 branch.

For the pending pull requests:

  • Apply code style in the branch
  • Rebase the main branch

Issue tracker

Automatic tests

  • Added test cases

Manual tests

  • Done

  • OS: LineageOS 20

Build tasks success

Successfully running following tasks on local:

  • ./gradlew assembledebug
  • ./gradlew spotlessCheck

@VishnuSanal
Copy link
Member

@Bambooin thanks a lot for the effort, but we have updated spotless (the library we use to enforce formatting) to the latest version without this humongous of a change (please see #3822). please check whether this would PR be a duplicate to that & LMK.

@Bambooin
Copy link
Contributor Author

Bambooin commented Nov 1, 2023

@VishnuSanal Sorry, I missed the #3822 which is very similar to this one.

This pull request only suppress one rule with standard:max-line-length in one file.

Other standard rules are enabled and fixed one by one manually.

Especially for the rule: standard:no-wildcard-imports this one should be enabled.

https://developer.android.com/kotlin/style-guide#import_statements

Wildcard imports (of any type) are not allowed.

This rule needs to be fixed by hand one by one which spends lots of time and effort.

IMO, we should follow Kotlin code conventions, and disable rules with reasonable purpose.

Such as the regex expression in NetCopyConnectionInfo.kt should supressed, but other rules should be kept.

This pull request was created after a myriad of pull requests were merged recently to avoid conflicts.

@VishnuSanal
Copy link
Member

@Bambooin i personally wouldn't vouch for a PR with ~350 file changes, since it breaks git history. I think the cost outweighs the benefits. this is why I created the other spotless PR before, it first gave me a whole lot of changes & (just like you did!) I had to try disabling individual rules manually to get to the current amount of files changed (~30 files). requesting for opinions from @VishalNehra @TranceLove & @EmmanuelMess here.

@Bambooin
Copy link
Contributor Author

Bambooin commented Nov 3, 2023

@VishnuSanal We can use git blame ignore file to skip this commit.

https://docs.github.com/en/repositories/working-with-files/using-files/viewing-a-file#ignore-commits-in-the-blame-view

For this pull request with too much change is not my git way too, we should keep each commit clean and elegant.

@Bambooin
Copy link
Contributor Author

Bambooin commented Nov 3, 2023

All CI check and test passed except code static anylysis failed with too long function.

@VishalNehra
Copy link
Member

Can you fix the static code analysis failure as well? Let me know if there's any rule that you would want to ignore for now, but we should be able to fix too long functions.

@Bambooin
Copy link
Contributor Author

Bambooin commented Nov 5, 2023

There are 9 functions are too lang which exceed maximum length 60.
https://github.com/TeamAmaze/AmazeFileManager/runs/18341863611

I tried and failed to refactor these too complex functions.

Such as this function is 199 lines with new code style.

app/src/main/java/com/amaze/filemanager/utils/MainActivityActionMode.kt#L245
The function onActionItemClicked is too long (199). The maximum length is 60.

@Bambooin
Copy link
Contributor Author

Close it due to it's hard to review and merge conflicts.

@Bambooin Bambooin closed this Nov 16, 2023
@VishalNehra
Copy link
Member

I was planning to merge it with the next release cycle after few days. There were always going to be merge conflicts here, so I'm trying to minimise them by making a release and then starting work on new release after merging this first.

@Bambooin Bambooin reopened this Nov 17, 2023
@Bambooin
Copy link
Contributor Author

Waiting for the new release and try to merge or rebaes(hard)...

@VishalNehra
Copy link
Member

Hi, we just published a new release. Can you please rebase and we'll push this to merge. Sorry it took so long.
While you're at it, can you please also update the licence year in template in license_amaze_strings as well.

@VishalNehra VishalNehra added this to the v4.0 milestone Feb 17, 2024
@VishalNehra
Copy link
Member

I'm not able to see your previous changes here now. Did you remove old commits?

@Bambooin
Copy link
Contributor Author

@VishalNehra The v1 patch is very hard to rebase and code review, so I rewrite the v2 patch from scratch.

Sorry for the long waiting time, it's ready to code review right now.

While you're at it, can you please also update the licence year in template in license_amaze_strings as well.

Does this mean the license header? I had updated the license year in the code base.

SLF4J: No SLF4J providers were found.
SLF4J: Defaulting to no-operation (NOP) logger implementation
SLF4J: See https://www.slf4j.org/codes.html#noProviders for further details.
SLF4J: Class path contains SLF4J bindings targeting slf4j-api versions 1.7.x or earlier.
SLF4J: Ignoring binding found at [jar:file:/home/runner/.gradle/caches/transforms-3/7adcb909ec6a171fc6a1a1c1cf301964/transformed/jetified-logback-classic-1.2.11.jar!/org/slf4j/impl/StaticLoggerBinder.class]
SLF4J: See https://www.slf4j.org/codes.html#ignoredBindings for an explanation.
This issue was introduced in: 7136ff3

Fix by removing old codes and keep the new codes.

> ISO 639 is not a stable standard; some of the language codes it defines (specifically "iw", "ji", and "in") have changed. This constructor accepts both the old codes ("iw", "ji", and "in") and the new codes ("he", "yi", and "id"), but all other API on Locale will return only the OLD codes.

com.amaze.filemanager.ui.fragments.preferencefragments.UiPrefsFragmentTest > testDefaultBehaviour[28] FAILED
    java.lang.IllegalArgumentException: list[49] is a repetition
        at android.os.LocaleList.__constructor__(LocaleList.java:193)
        at android.os.LocaleList.<init>(LocaleList.java)
        at androidx.core.os.LocaleListCompat$Api24Impl.createLocaleList(LocaleListCompat.java:341)
        at androidx.core.os.LocaleListCompat.create(LocaleListCompat.java:77)
        at androidx.core.os.LocaleListCompat.forLanguageTags(LocaleListCompat.java:172)
        at com.amaze.filemanager.utils.ContextLocaleExtKt.getLocaleListFromXml(ContextLocaleExt.kt:53)
        at com.amaze.filemanager.utils.ContextLocaleExtKt.getLangPreferenceDropdownEntries(ContextLocaleExt.kt:62)
        at com.amaze.filemanager.ui.fragments.preferencefragments.UiPrefsFragment.onCreatePreferences(UiPrefsFragment.kt:55)
        at androidx.preference.PreferenceFragmentCompat.onCreate(PreferenceFragmentCompat.java:161)
        at androidx.fragment.app.Fragment.performCreate(Fragment.java:3090)
@VishalNehra VishalNehra merged commit 1edfe54 into TeamAmaze:release/4.0 Mar 30, 2024
2 of 3 checks passed
@VishnuSanal VishnuSanal mentioned this pull request Jun 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants