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

Detekt: fix end period #6064

Merged
merged 4 commits into from
May 16, 2022
Merged

Detekt: fix end period #6064

merged 4 commits into from
May 16, 2022

Conversation

bmarty
Copy link
Member

@bmarty bmarty commented May 16, 2022

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other : documentation and code quality

Content

Add missing period in Kdoc. Enable rule EndOfSentenceFormat

Will require matrix-org/matrix-analytics-events#63

Motivation and context

Improve code quality and generated documentation quality

Reviewers

Sorry for the big PR. There is no code change hopefully. Maybe using https://patch-diff.githubusercontent.com/raw/vector-im/element-android/pull/6064.diff can help. Maybe not.

@bmarty bmarty requested review from a team, Claire1817 and ericdecanini and removed request for a team May 16, 2022 08:59
@github-actions
Copy link

github-actions bot commented May 16, 2022

Unit Test Results

122 files  122 suites   2m 6s ⏱️
205 tests 205 ✔️ 0 💤 0
690 runs  690 ✔️ 0 💤 0

Results for commit 12eb23b.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@ouchadam ouchadam left a comment

Choose a reason for hiding this comment

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

LGTM 👍 (Didn't check every file...)

Copy link
Contributor

@fedrunov fedrunov left a comment

Choose a reason for hiding this comment

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

I've checked every file and I feel like I need a pair of new eyes now :) There is one place, where I think dot might be missing and small NIT (not important thing) which I think doesn't deserve to make a commit just for it

* {
* "jitsi" : {
* "jitsi.domain.org" : true,
* "jitsi.other.org" : false
* }
* }
* </pre>
Copy link
Contributor

@fedrunov fedrunov May 16, 2022

Choose a reason for hiding this comment

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

Missing dot here? Or it's not required when sentence starts and ends with the same tag

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is OK for detekt since the first sentence Map of native widgetType to a map of domain to Allowed. ends with a period.

Copy link
Contributor

Choose a reason for hiding this comment

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

I just thought that every sentence in doc have to end with dot, since you actually do so in other places, with many sentences, like here:

https://github.com/vector-im/element-android/blob/12eb23b198254191274601f0f933ceb67c7cb251/matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/query/QueryStringValue.kt#L50

It doesn't matter much, if it's ok for detekt, just curious

Copy link
Contributor

@fedrunov fedrunov left a comment

Choose a reason for hiding this comment

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

LGTM

@bmarty bmarty merged commit 3674ae7 into develop May 16, 2022
@bmarty bmarty deleted the feature/bma/detekt_end_period branch May 16, 2022 12:24
@github-actions
Copy link

Matrix SDK

Integration Tests Results:

  • [org.matrix.android.sdk.session]
    = passed=7 failures=13 errors=0 skipped=3
  • [org.matrix.android.sdk.account]
    = passed=1 failures=2 errors=0 skipped=2
  • [org.matrix.android.sdk.internal]
    = passed=88 failures=59 errors=0 skipped=13
  • [org.matrix.android.sdk.ordering]
    = passed=16 failures=0 errors=0 skipped=0
  • [org.matrix.android.sdk.PermalinkParserTest]
    = passed=2 failures=0 errors=0 skipped=0

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.

3 participants