gaelmarhic / Quadrant

A Gradle plugin for Android that makes navigation easy in multi-module projects.
Apache License 2.0
216 stars 13 forks source link

Support partial name for Activities #4

Closed marcellogalhardo closed 3 years ago

marcellogalhardo commented 3 years ago

Quadrant supports only full qualified name, failing the build preemptively if a partial name is found.

As proposed by @IslamSalah in Pull Request #3, as Android supports partial names in their Manifest, it would be a nice feature to be added to ease adoption of the library for codebases with many modules that are using this standard.

I'm creating this issue to discuss design decisions around the Pull Request #3, possible limitations and risks.

marcellogalhardo commented 3 years ago

Thank you @IslamSalah for your taking the time and initiative to open a Pull Request for this functionality. @gael and I chatted few times inside N26 about supporting partial Activity names. Still, it was, at the time, a design decision to keep only full qualified class names to reduce corner cases, reduce the chance of problems, and lastly, make the library easier to maintain.

For context, the production app we are talking here has a huge code base with 300+ modules that uses Quadrant as the foundation to our custom navigation system, and we had a legacy part of our system that would have very problematic package names (yep, our fault!) and full qualified names were the safest option. That said, I think your proposal is very valid, but I am very interested to understand some of the statements you did in your Pull Request #3 if you don't mind.

With relaxing some constraints, it can be even easier to use.

What do you mean here with "easier to use"? Do you mean easy in a sense it would support both cases, and people adopting the library would not need to change their manifests?

Especially for big-sized teams where applying constraints can add some friction.

What do you mean by friction? Is that a technical friction (to change multiple modules) or people friction (where people have different opinions and don't agree which one to use)? Do you mind sharing how many modules we are talking about and what are problems you are facing with it?

IslamSalah commented 3 years ago

Thank you for starting the discussion

The main goal of supporting PQCN is removing any cognitive load on the team contributors. So if the implementation details are behind a decent layer of abstraction, contributors needn't know about following the FQCN constraint or even know about the plugin

I believe the main reason the plugin supports only FQCN is to guarantee compile time safety. However I have 2 comments here:

Also when @gaelmarhic can get a chance to look into it, he can share his concerns

marcellogalhardo commented 3 years ago

@IslamSalah thank you for the clarification.

Yes, the goal was to reduce the chance of human mistake in the navigation system.

Anyway, your proposal also matches how Android handles it. From Android Documentation: signal-2021-02-20-175112.png

What do you think @gaelmarhic?

gaelmarhic commented 3 years ago

Thank you @IslamSalah for bringing up this idea and thank you Marcello for taking part in the conversation! @IslamSalah You are actually right. I just checked and, even without Quadrant, and therefore using explicit class names, if an activity is not properly defined in the Manifest, it will crash at runtime! We can proceed with your idea. I will review the code now :)

gaelmarhic commented 3 years ago

The corresponding PR has been merged!