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

Support activity partially qualified class names #3

Conversation

IslamSalah
Copy link
Contributor

@IslamSalah IslamSalah commented Feb 7, 2021

Overview:

This plugin adds a tangible benefit for multi-module navigation. With relaxing some constrains it can be even easier to use. Especially for big-sized teams where applying constrains can add some friction. Currently the plugin fails the build with guiding error on breaking the FQCN constraint. On relaxing this constrains, things go smoother and siliently pass
CC @gaelmarhic

Type of change:

Feature

What's the purpose of this PR?

Extend the plugin capability to handle both partially & fully qualified class names for manifest files defined activities

What's the impact of this PR?

  • Support partially qualified class names
<manifest xmlns:android="http://schemas.android.com/apk/res/android"
    package="com.poc.bookingfeature">

    <application>
        <activity android:name=".BookingActicity" />
...
  • Retain support for fully qualified class names
<manifest xmlns:android="http://schemas.android.com/apk/res/android"
    package="com.poc.bookingfeature">

    <application>
        <activity android:name="com.poc.bookingfeature.BookingActicity" />
...

N.B commit by commit review can be helpful

@gaelmarhic
Copy link
Owner

Thanks so much for your contribution @IslamSalah !
I have quite a busy week, but I will definitely have a look at it this week-end and get back to you!
Cheers :)

Copy link
Owner

@gaelmarhic gaelmarhic left a comment

Choose a reason for hiding this comment

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

Thank you @IslamSalah once again for your contribution!
Your idea is great and its implementation is really well done!
I just added a few nitpicky comments.
Please review them and let me know what you think!
Cheers :)

README.md Show resolved Hide resolved
@@ -56,6 +59,24 @@ class ActivityFilteringHelper(
)
}

private fun List<ParsedManifest>.generateFullyQualifiedClassNames(): List<ParsedManifest> {
Copy link
Owner

Choose a reason for hiding this comment

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

Could we go with another name for this function? It does not "generate" a fully qualified class name all the time since some Activities are already declared that way. Could we have something like "formatClassNames" or similar?

Copy link
Owner

Choose a reason for hiding this comment

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

Also, I know I am a bit of a maniac, but could you put your 2 new functions above the "filterClassNames" one? To make the file more readable I tried to order the functions by order of appearance :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does not "generate" a fully qualified class name all the time since some Activities are already declared that way

IMO it's fine, similar to List#clear which doesn't clear the list if it's already cleared but its output is always a cleared list

Copy link
Owner

Choose a reason for hiding this comment

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

Fair enough!

Copy link
Owner

@gaelmarhic gaelmarhic left a comment

Choose a reason for hiding this comment

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

LGTM!

@gaelmarhic gaelmarhic merged commit bef47c5 into gaelmarhic:master Feb 21, 2021
@IslamSalah
Copy link
Contributor Author

Thank you for the review, should we expect a new release soon?

@gaelmarhic
Copy link
Owner

Just released version 1.5 including this change:
https://github.com/gaelmarhic/Quadrant/releases
https://plugins.gradle.org/plugin/com.gaelmarhic.quadrant
Let me know how it goes and thanks again ;)

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.

2 participants