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

Refactor large files into 1 file per class #138

Merged
merged 20 commits into from Aug 16, 2021
Merged

Conversation

ben-xD
Copy link
Contributor

@ben-xD ben-xD commented Jul 7, 2021

The main objective of this PR was to move classes (& enums) that were in the same file into their own separate file. Once I did that though, each file had to be exported via ably_flutter.dart, and that was not clean, ably_flutter would have had to contain a list of all files. The solution taken was to create separate mini libraries with their own src directory, and export the source files from these packages. Then inside ably_flutter.dart, these libraries will be exported instead of each file. I took the opportunity to move files into more logical folders. Other small changes are present: typo fix, unused import removal, simplify test teardown code.

This is a pattern observed in the getx repo, though if another pattern is cleaner, please mention it.

This PR depends on #136

Notes for the future: There are additional patterns we can learn/ follow in the future, and this refactor would help with this. Another improvement we can make is to create an internal platform library, and not expose these in the public API like we do currently. It also makes the transition to a full Dart SDK in the future easier too. For example, renaming Rest to RestNative, and changing the public API to rely on an abstract factory. We can allow the user to use either the Native or the Dart SDK.

I was worried about public API changes, and I don't think this PR affects it. As long as only public APIs/ classes are added to the library files (e.g. ably_flutter.dart, push_notifications.dart, logging.dart). Theres more about this in the documentation: "Code under lib/src is considered private; other packages should never need to import src/.... To make APIs under lib/src public, you can export lib/src files from a file that’s directly under lib.", and on a stackoverflow answer.

@github-actions github-actions bot temporarily deployed to staging/pull/138/dartdoc July 7, 2021 16:23 Inactive
…e dart analysis warning: Only reference in scope identifiers in doc comments.
@ben-xD ben-xD force-pushed the refactor-classes-into-files branch from 29da6aa to 3dcd2f7 Compare July 7, 2021 16:37
@github-actions github-actions bot temporarily deployed to staging/pull/138/dartdoc July 7, 2021 16:39 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/138/dartdoc July 8, 2021 13:00 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/138/dartdoc July 8, 2021 13:19 Inactive
@ben-xD ben-xD force-pushed the refactor-classes-into-files branch from e0a7836 to 915b28f Compare July 8, 2021 13:19
@github-actions github-actions bot temporarily deployed to staging/pull/138/dartdoc July 8, 2021 13:22 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/138/dartdoc July 9, 2021 08:12 Inactive
@ben-xD
Copy link
Contributor Author

ben-xD commented Jul 9, 2021

Android 19 integration tests are failing, but after mentioning it in the stand up, we are actually dropping support for Android 19 and 20, raising minimum API level to 21. Please see Review JDK 7 and Android API Level requirements for Java SDK for more information.

@github-actions github-actions bot temporarily deployed to staging/pull/138/dartdoc July 9, 2021 12:16 Inactive
@ben-xD ben-xD requested review from tiholic and QuintinWillison and removed request for tiholic August 5, 2021 08:40
Copy link
Contributor

@QuintinWillison QuintinWillison left a comment

Choose a reason for hiding this comment

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

Very difficult to review, given the size of change and amount of movement.

I think, in the interests of getting this landed sooner rather than later, it's best to rely on tests to indicate whether it still works. Also example app as proof of life.

Copy link
Contributor

@tiholic tiholic left a comment

Choose a reason for hiding this comment

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

Looks good overall, few comments:

lib/src/authentication/src/auth.dart Show resolved Hide resolved
lib/src/platform/platform.dart Show resolved Hide resolved
lib/src/platform/src/platform.dart Show resolved Hide resolved
lib/src/rest/src/options.dart Outdated Show resolved Hide resolved
lib/src/stats/stats_internal_granularity.dart Outdated Show resolved Hide resolved
test/mock_method_call_manager.dart Outdated Show resolved Hide resolved
Base automatically changed from feature/nullsafety to main August 12, 2021 12:33
@ben-xD ben-xD changed the base branch from main to schedule-integration-test-workflow August 12, 2021 12:33
@ben-xD ben-xD changed the title Refactor classes into files Refactor large files into 1 files per class Aug 12, 2021
@github-actions github-actions bot temporarily deployed to staging/pull/138/dartdoc August 12, 2021 12:38 Inactive
@ben-xD ben-xD changed the title Refactor large files into 1 files per class Refactor large files into 1 file per class Aug 12, 2021
@github-actions github-actions bot temporarily deployed to staging/pull/138/dartdoc August 13, 2021 13:18 Inactive
Base automatically changed from schedule-integration-test-workflow to main August 15, 2021 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants