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

Sdk user story #5952

Merged
merged 22 commits into from May 30, 2022
Merged

Sdk user story #5952

merged 22 commits into from May 30, 2022

Conversation

bmarty
Copy link
Member

@bmarty bmarty commented May 5, 2022

This PR add some cleanup on the SDK documentation and on the API. See commit messages for the details.

Also it introduced the SDK user Story, with their documentation:

image

and for Us100SignIn for instance:

image

This is still WIP, but we can merge as it is and update later, to first check that the format is OK.

@github-actions
Copy link

github-actions bot commented May 5, 2022

Unit Test Results

124 files  ±  0  124 suites  ±0   2m 1s ⏱️ -1s
220 tests +  3  220 ✔️ +  3  0 💤 ±0  0 ±0 
726 runs  +12  726 ✔️ +12  0 💤 ±0  0 ±0 

Results for commit 8f06415. ± Comparison against base commit 109b381.

This pull request removes 1 and adds 4 tests. Note that renamed tests count towards both.
im.vector.app.features.onboarding.ftueauth.FtueMissingRegistrationStagesComparatorTest ‑ when ordering stages, then prioritizes email
im.vector.app.features.onboarding.RegistrationActionHandlerTest ‑ given adding an email ThreePid fails with 401, when handling register action, then infer EmailSuccess
im.vector.app.features.onboarding.RegistrationActionHandlerTest ‑ given email verification errors with 401 and succeeds, when checking email validation, then continues to poll until success
im.vector.app.features.onboarding.RegistrationActionHandlerTest ‑ given email verification errors with 401 then fatal error, when checking email validation, then continues to poll until non 401 error
im.vector.app.features.onboarding.ftueauth.MatrixOrgRegistrationStagesComparatorTest ‑ when ordering stages, then prioritizes email

♻️ This comment has been updated with latest results.

@bmarty bmarty force-pushed the feature/bma/sdk_user_story branch from 322937a to d09acce Compare May 11, 2022 13:06
@bmarty bmarty marked this pull request as ready for review May 11, 2022 13:17
@bmarty bmarty requested review from a team, yostyle and mnaturel and removed request for a team May 11, 2022 13:17
get() = keysBackupVersion?.version

fun isEnabled(): Boolean
fun isStucked(): Boolean
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it should be isStuck() instead. The version with "ed" seems to not exist.

Copy link
Contributor

Choose a reason for hiding this comment

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

When renaming a method which is exposed as an API, should we add an entry in the changelog?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yes, will add.

Copy link
Member Author

Choose a reason for hiding this comment

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

@@ -1,3 +1,7 @@
# Package org.matrix.android.sdk._userstories
Copy link
Contributor

Choose a reason for hiding this comment

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

The "_" should be removed since you renamed it without it.

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, thanks. Sadly Dokka does not complain :/

Copy link
Member Author

Choose a reason for hiding this comment

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

With the fix, we can see that the package description is back (to compare with the screenshot in this PR description):

image

* ### Required APIs:
* - [org.matrix.android.sdk.api.Matrix] constructor
*/
object Us000InitMatrix
Copy link
Contributor

Choose a reason for hiding this comment

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

I have 2 questions:

  • do the user stories really need to be object? I think simple class could work and avoid loading of non used US at runtime.
  • Am I missing something or the US are not implemented yet? If not implemented, would't be better to make it internal or at least mention it in the doc?

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 point. Yes this could be class, that's better. Also maybe with a private constructor. Those classes are just here to be visible in the documentation.
This is not ideal, if you have a better idea...

@bmarty bmarty requested a review from mnaturel May 12, 2022 12:59
Copy link
Contributor

@mnaturel mnaturel 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 just added a comment about whether we should add changelog entry for the renaming of the isStuck method.

@bmarty bmarty force-pushed the feature/bma/sdk_user_story branch from 901d339 to eb57680 Compare May 18, 2022 14:25
@bmarty bmarty force-pushed the feature/bma/sdk_user_story branch from eb57680 to 53c83ab Compare May 20, 2022 19:23
Comment on lines +21 to +23
/**
* Either a session or an object containing data about registration stages.
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

When you have a basic documentation, you could replace by a single line comment. This avoids overloading the file with comment lines

Suggested change
/**
* Either a session or an object containing data about registration stages.
*/
/** Either a session or an object containing data about registration stages. */

Copy link
Member Author

Choose a reason for hiding this comment

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

This is definitely more compact. I will think about it for next changes.
With the current format, it's easier to add new lines.

@bmarty bmarty merged commit ae94f45 into develop May 30, 2022
@bmarty bmarty deleted the feature/bma/sdk_user_story branch May 30, 2022 16:30
@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=23 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