Skip to content
This repository has been archived by the owner on Feb 20, 2023. It is now read-only.

For #26844: Update ktlint to 0.47.0. #26846

Merged
merged 6 commits into from
Sep 8, 2022
Merged

Conversation

mcarare
Copy link
Contributor

@mcarare mcarare commented Sep 6, 2022

Pull Request checklist

  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not
  • Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features. In addition, it includes a screenshot of a successful accessibility scan to ensure no new defects are added to the product.

QA

  • QA Needed

To download an APK when reviewing a PR (after all CI tasks finished running):

  1. Click on Checks at the top of the PR page.
  2. Click on the firefoxci-taskcluster group on the left to expand all tasks.
  3. Click on the build-debug task.
  4. Click on View task in Taskcluster in the new DETAILS section.
  5. The APK links should be on the right side of the screen, named for each CPU architecture.

GitHub Automation

Fixes #26844

Sorry, something went wrong.

@mcarare mcarare added the pr:work-in-progress PRs that are not ready to be reviewed yet and are actively being worked on label Sep 6, 2022
@gabrielluong
Copy link
Member

Hmm.. given how large the log is for the failing trailing comma lint, I think we probably want to just go with adding a baseline as well.

@mcarare mcarare force-pushed the 26844 branch 4 times, most recently from 9ddccf1 to 28f46e9 Compare September 7, 2022 09:30
@mcarare mcarare added needs:review PRs that need to be reviewed and removed pr:work-in-progress PRs that are not ready to be reviewed yet and are actively being worked on labels Sep 7, 2022
@mcarare mcarare marked this pull request as ready for review September 7, 2022 10:39
@mcarare mcarare requested review from a team as code owners September 7, 2022 10:39
.editorconfig Outdated
ij_kotlin_allow_trailing_comma=true

ktlint_disabled_rules=import-ordering
Copy link
Member

Choose a reason for hiding this comment

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

Let's add a new line to EOF

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we want to take this opportunity to automatically insert a new line to EOF?

It is as simple as adding insert_final_newline = true in this editorconfig file.

Copy link
Member

@gabrielluong gabrielluong Sep 7, 2022

Choose a reason for hiding this comment

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

Yes, let's do that. We'll have to update that for AC and Focus.

@gabrielluong gabrielluong added pr:approved PR that has been approved and removed needs:review PRs that need to be reviewed labels Sep 7, 2022
.editorconfig Outdated
ij_kotlin_allow_trailing_comma=true

ktlint_disabled_rules=import-ordering
Copy link
Member

@gabrielluong gabrielluong Sep 7, 2022

Choose a reason for hiding this comment

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

Can you also move this back to where it's associated comment are on line 2-3

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, thank you for noticing this, I inserted the new rules in a wrong place by mistake.

@@ -70,7 +70,7 @@ private data class PopupHorizontalBounds(
* @param action Optional other composable to show just below the popup text.
*/
@SuppressLint("ViewConstructor") // Intended to be used only in code, don't need a View constructor
internal class CFRPopupFullScreenLayout(
internal class CFRPopupFullscreenLayout(
Copy link
Member

Choose a reason for hiding this comment

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

This is an aside thought, but do we know why the ktlint in CI didn't complain and block the initial merge when the file name rule failed for this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From the changelog it seems the rule has been added in 0.46.0. (It was later modified in 0.47.0 to allow different naming for files with just a single top-level declaration of type function - especially in the case of extension functions).

I do remember similar warnings about file names, but that could have been from lint or IDE (?)

@mcarare mcarare added pr:needs-landing PRs that are ready to land [Will be merged by Mergify] and removed pr:approved PR that has been approved labels Sep 8, 2022
@mcarare mcarare force-pushed the 26844 branch 2 times, most recently from 59b83c7 to 0abd30b Compare September 8, 2022 10:25
… end of each file.

This automatically adds a new line at EOF when there is none.
@mergify mergify bot merged commit ca88c8c into mozilla-mobile:main Sep 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pr:needs-landing PRs that are ready to land [Will be merged by Mergify]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update ktlint to 0.47.0
2 participants