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

Ktlint upgrade lint fix #1242

Merged
merged 33 commits into from
Jun 4, 2020
Merged

Ktlint upgrade lint fix #1242

merged 33 commits into from
Jun 4, 2020

Conversation

vinitamurthi
Copy link
Contributor

Explanation

The new Ktlint (version 0.37) is expecting imports to be formatted alphabetically but it will group java and javax imports together.
So you would have normal imports (alphabetically), then imports starting with java (alphabetically), and then imports starting with javax (alphabetically). Right now our linters are failing because we did it alphabetically without any grouping. Creating this PR to demonstrate what the new change would be and get your thoughts on whether we should go forward with
this?

Checklist

  • The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • The PR explanation includes the words "Fixes #bugnum: ..." (or "Fixes part of #bugnum" if the PR only partially fixes an issue).
  • The PR follows the style guide.
  • The PR does not contain any unnecessary auto-generated code from Android Studio.
  • The PR is made from a branch that's not called "develop".
  • The PR is made from a branch that is up-to-date with "develop".
  • The PR's branch is based on "develop" and not on any other branch.
  • The PR is assigned to an appropriate reviewer in both the Assignees and the Reviewers sections.

Copy link
Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @vinitamurthi!

@BenHenning BenHenning assigned vinitamurthi and unassigned BenHenning Jun 3, 2020
Copy link
Contributor

@rt4914 rt4914 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have left one comment.

Also, regarding imports, I am fine with this formatting as earlier also it was like this only. It would be nice to have everything alphabetical but if thats too much work, this will also work easily.

@rt4914 rt4914 removed their assignment Jun 4, 2020
Copy link
Contributor

@anandwana001 anandwana001 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems it worked.

@rt4914 rt4914 assigned vinitamurthi and unassigned rt4914 Jun 4, 2020
@vinitamurthi vinitamurthi merged commit 929624d into develop Jun 4, 2020
@vinitamurthi vinitamurthi deleted the ktlint_upgrade branch June 4, 2020 11:34
Sarthak2601 pushed a commit that referenced this pull request Jun 5, 2020
* Fixed new ktlint errors

* Fixed data module lint errors with new ktlint

* Add memory param to JAVA_TOOL_OPTIONS

* Add gradle options as well

* Remove java_tool_options

* Reduce test threads and increase ram of docker

* Fix docker image issue

* Removed large resource class since its not available for our plan

* Remove test fork config from gradle files and added it to the test command

* Attempt to build the app before running tests

* Build the app without any daemon/worker restrictions

* Test a new setup job on circleci

* Change indentation of config

* Fix setup

* Fix ktlint installation

* Revert android dependencies

* Fix unterminated string issue

* Add workspace attachment

* Fix workspace path

* Fix setup path

* Separate linters from the android dependencies

* Fix cache restore issue

* Split non flaky tests into module wise test jobs

* Set heap size in app tests

* Fixed wrong repo

* Testing changing java version'

* Add the command instead of an orb

* Update java home path

* Fix java home path

* Add java 11 installation to every job for verification

* Remove JDK 11 and enable build caching

* Reduce size for each daemon

* Remove splash tests from the non flaky app tests
Sarthak2601 added a commit that referenced this pull request Jun 12, 2020
* config file

* dummy data

* fake string

* Ktlint upgrade lint fix (#1242)

* Fixed new ktlint errors

* Fixed data module lint errors with new ktlint

* Add memory param to JAVA_TOOL_OPTIONS

* Add gradle options as well

* Remove java_tool_options

* Reduce test threads and increase ram of docker

* Fix docker image issue

* Removed large resource class since its not available for our plan

* Remove test fork config from gradle files and added it to the test command

* Attempt to build the app before running tests

* Build the app without any daemon/worker restrictions

* Test a new setup job on circleci

* Change indentation of config

* Fix setup

* Fix ktlint installation

* Revert android dependencies

* Fix unterminated string issue

* Add workspace attachment

* Fix workspace path

* Fix setup path

* Separate linters from the android dependencies

* Fix cache restore issue

* Split non flaky tests into module wise test jobs

* Set heap size in app tests

* Fixed wrong repo

* Testing changing java version'

* Add the command instead of an orb

* Update java home path

* Fix java home path

* Add java 11 installation to every job for verification

* Remove JDK 11 and enable build caching

* Reduce size for each daemon

* Remove splash tests from the non flaky app tests

* Fixes #1084: Tablet: Highfi Onboarding Flow (landscape) (#1115)

* -Onboarding: draft implementation for tablet-landscape high-fi

* -Removed the unused drawable file
-Used `app:layout_constraintDimensionRatio="H, 10:9"` and `android:scaleType="fitXY"` instead of `guideline` and `android:scaleType="centerCrop"`
-Centered the `textViews` between the `imageView` and the end of the `parentLayout` and gave them a fixed width of `360dp` instead of placing the `textViews` relative to the end of the `parentLayout` only

* -placed the textViews and the buttons relative to the end of the screen (with a margin of 96dp) and a guideline of 57% from the beginning of the screen instead of using fixed width of 360dp

* -Deleted "onboarding_dot_active.xml" with "sw600dp-land" qualifier
-Removed the unnecessary "FrameLayout" in "onboarding_fragment.xml"
-Removed the "guidelines" in "onboarding_slide.xml" and "onboarding_slide_final.xml" and replaced them with margins and fixed widths
-Updated the "oppiaOnboardingDivider" in "colors.xml" and used it in the "onboarding_fragment.xml"

* -Updated the UI

* -Added EOF in "booleans.xml"

* -Updated the ratio of the thumbnail images to be "40:39"

* -Changed the height of the "Get Started" button to be "wrap_content" (minHeight is set to 48dp)

* Fixes #1216 : Deleted Extra Proto Models (#1232)

* Remove Extra Proto Models

* try gradle changes

* try circle ci changes

* remove testing changes[skip CI]

* Fixes #1064 : Added DropDropSortInput Module (#1092)

* Add DropDropSortInput Module along with HasElementXwithPositionY Classifier

* Fixes #1221 : Added HasElementXBeforeElementY Classifier Rule [Blocked: #1092] (#1231)

* Add DragDropSortInputHasElementXBeforeElementYClassifierProvider

* dummy

* dev project config file

* nit

Co-authored-by: Vinita Murthi <murthi.vinita@gmail.com>
Co-authored-by: Mohamed Medhat <abomed7at55@gmail.com>
Co-authored-by: Pulkit Aggarwal <aggarwalpulkit596@gmail.com>
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.

4 participants