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

Feature/bma/detekt outdated doc #6084

Merged
merged 6 commits into from
May 20, 2022
Merged

Conversation

bmarty
Copy link
Member

@bmarty bmarty commented May 18, 2022

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Content

Enable the rule OutdatedDocumentation and fix all existing error reported by Detekt.

The goal is not here to write the best documentation of the existing code, but is to ensure that Kdoc and documentation will not get worse.

Sometime @param name was not correct, sometimes it some @param was missing, sometimes not in the correct order, sometime @param was used instead of @property.

Note that this rule does not complain if there is no @param in the Kdoc.

Motivation and context

Improve documentation quality, especially SDK API documentation

Screenshots / GIFs

N/A

Tests

N/A

@bmarty bmarty requested review from a team, onurays and ariskotsomitopoulos and removed request for a team May 18, 2022 08:43
@@ -90,7 +91,7 @@ interface PermalinkService {
* Creates a HTML or Markdown mention span template. Can be used to replace a mention with a permalink to mentioned user.
* Ex: "<a href=\"https://matrix.to/#/%1\$s\">%2\$s</a>" or "[%2\$s](https://matrix.to/#/%1\$s)"
*
* @param type: type of template to create
* @param type type of template to create
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 have also removed some unnecessary :, but Detekt does not report that.

@bmarty
Copy link
Member Author

bmarty commented May 18, 2022

Thanks GitHub for having picked @ariskotsomitopoulos up to review this PR: I have added some documentation on the API about threads, I am not sure it is accurate. Maybe not update the doc here, but if you want to create dedicated PR to complete the doc about thread (especially in the Matrix SDK API), it will be appreciated. (only once this PR's been merged)

@github-actions
Copy link

github-actions bot commented May 18, 2022

Unit Test Results

122 files  ±0  122 suites  ±0   2m 10s ⏱️ +8s
205 tests ±0  205 ✔️ ±0  0 💤 ±0  0 ±0 
690 runs  ±0  690 ✔️ ±0  0 💤 ±0  0 ±0 

Results for commit e2b77d5. ± Comparison against base commit 6c8e047.

♻️ This comment has been updated with latest results.

@bmarty bmarty force-pushed the feature/bma/detekt_outdated_doc branch from 25ccaaa to 805d2d2 Compare May 18, 2022 11:16
Copy link
Contributor

@ariskotsomitopoulos ariskotsomitopoulos left a comment

Choose a reason for hiding this comment

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

Approved. Some minor comments, I will create a separate PR to improve threads doc as we discussed

@@ -46,8 +46,8 @@ class MXUsersDevicesMap<E> {
/**
* Provides the object for a device id and a user Id.
*
* @param deviceId the device id
* @param userId the object id
Copy link
Contributor

Choose a reason for hiding this comment

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

should be * @param userId the user id instead of * @param userId the object id

Copy link
Member Author

Choose a reason for hiding this comment

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

good catch yes.

@@ -99,6 +99,7 @@ interface IntegrationManagerService {
* Offers to allow or disallow a native widget domain.
* @param widgetType the widget type to check for
* @param domain the domain to check for
* @param allowed true or false
Copy link
Contributor

Choose a reason for hiding this comment

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

* @param allowed true or false could be a bit more detailed

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 agree, ha ha

@@ -65,7 +65,8 @@ interface WidgetPostAPIMediator {
/**
* Send an object response.
*
* @param klass the class of the response
* @param T the generic type
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if * @param T the generic type is needed

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, it has to be documented. The doc does not bring a lot TBH.

@@ -140,7 +140,7 @@ internal object MXMegolmExportEncryption {
*
* @param data the data to encrypt.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: we don't need those spaces.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, we should do a check on the whole codebase

Copy link
Member Author

Choose a reason for hiding this comment

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

done

* @param baseType The base type for which this factory will create adapters. Cannot be Object.
* @param labelKey The key in the JSON object whose value determines the type to which to map the
* JSON object.
* @param fallbackType
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe somethink like * @param fallbackType alternative Type to try in case of the serialization fails

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks, will add.

Copy link
Contributor

@onurays onurays left a comment

Choose a reason for hiding this comment

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

Very nice clean up! Please see my suggestion about the parameter without any description.

@bmarty bmarty force-pushed the feature/bma/detekt_outdated_doc branch from e2b77d5 to f5d0663 Compare May 20, 2022 07:48
@bmarty
Copy link
Member Author

bmarty commented May 20, 2022

Rebased (BadgeProxy.kt has been removed)

@bmarty bmarty enabled auto-merge May 20, 2022 07:48
@bmarty bmarty disabled auto-merge May 20, 2022 10:06
@bmarty bmarty merged commit 4094a66 into develop May 20, 2022
@bmarty bmarty deleted the feature/bma/detekt_outdated_doc branch May 20, 2022 10:06
@bmarty
Copy link
Member Author

bmarty commented May 20, 2022

I do not know why GA are blocked. Let's merge, CI was happy before.

@github-actions
Copy link

Matrix SDK

Integration Tests Results:

  • [org.matrix.android.sdk.session]
    = passed=20 failures=0 errors=0 skipped=3
  • [org.matrix.android.sdk.account]
    = passed=3 failures=0 errors=0 skipped=2
  • [org.matrix.android.sdk.internal]
    = passed=57 failures=1 errors=0 skipped=1
  • [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.

None yet

3 participants