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

Baseline for needs migration #203

Open
Laimiux opened this issue Dec 1, 2022 · 8 comments
Open

Baseline for needs migration #203

Laimiux opened this issue Dec 1, 2022 · 8 comments

Comments

@Laimiux
Copy link

Laimiux commented Dec 1, 2022

This is a feature suggestion that could help existing projects that have many violations. Instead of having big aliases such as NeedsMigration, I wonder if we could have violation baseline which we can generate (similarly to detekt/lint)

@jraska
Copy link
Owner

jraska commented Dec 2, 2022

Hey, thanks for the suggestion.

Please what are your thoughts and motivation where would you find this useful? :)

@Laimiux
Copy link
Author

Laimiux commented Dec 2, 2022

Let's say we have a project with impl and public(called api in one of your talks) types of modules. When migrating an existing codebase to this structure there will be cases that break the categorization rules and need to be migrated over time. The NeedsMigration alias mentioned in the docs can help, but it's not fine-grained enough - once you mark a module with it, new violations can be added without triggering a failure.

Instead, I'm proposing a violation baseline where you can record all the existing violations.

{
    ":module-a:impl": [
        ":module-y:impl"
    ],
    "module-b:impl:": [
        ":module-y:impl",
    ],
}

This way we can fail the build if new violations are added (ex: :module-c:impl includes :module-y:impl)

@jraska
Copy link
Owner

jraska commented Dec 6, 2022

Thanks for sharing more context :) it is a similar topic to what was discussed at: #157.

Using more entries within the allowed array as a whitelist would not work for your case?
Like allowed = [:module-a:impl -> module-y:impl, module-b:impl -> module-y:impl].

Another option I can share is that the plugin can be used on each module, not only at the :app module so another option is using more granular allowed/restricted in each particular module. -the plugin can be applied for each module subgraph.

Basically I'm trying to see the gaps which are truly there and the ones which can be done by current feature set as I try to keep the feature set minimal to keep the plugin lean. :)

@Laimiux
Copy link
Author

Laimiux commented Dec 19, 2022

Thanks for sharing more context :) it is a similar topic to what was discussed at: #157.

Indeed, it's a very similar issue.

Using more entries within the allowed array as a whitelist would not work for your case? Like allowed = [:module-a:impl -> module-y:impl, module-b:impl -> module-y:impl].

Another option I can share is that the plugin can be used on each module, not only at the :app module so another option is using more granular allowed/restricted in each particular module. -the plugin can be applied for each module subgraph.

Basically I'm trying to see the gaps which are truly there and the ones which can be done by current feature set as I try to keep the feature set minimal to keep the plugin lean. :)

I was able to get a more fine-grained allowed ruleset to work, however it was a bit tricky with some custom logic and then labor intensive to manually write each violation. I think having violation baseline generated by the plugin would have been valuable.

@Laimiux Laimiux closed this as completed Jan 9, 2023
@jraska
Copy link
Owner

jraska commented Jan 17, 2023

Hey @Laimiux, sorry for not getting back to you earlier - end of the year was busy.

One option I was investigating is to use the error message as the simple way to generate the rules as when there is an error, all violations are printed.

E.g.

... GradleException: [':module-a:impl' -> ':module-y:impl', ':module-b:impl' -> ':module-y:impl'] not allowed...

Though it has to be modified as the format required would be allowed = [':module-a:impl -> :module-y:impl', ':module-b:impl -> :module-y:impl']

So not ideal, but some start 🤔

Anyway about the whole feature you ask for I feel like I won't be able to get to this soon and probably the preferred solution would be unifying the error message format with the configuration so there is a simple way of copy/paste and use this as the baseline.

Thanks a lot for raising the issue in any case :)

@Laimiux
Copy link
Author

Laimiux commented Oct 16, 2024

I was able to work around this using a hacky solution, but it's becoming harder to scale this. The hacky solution to enable fine-grained violations requires us to define moduleNameAssertAlias dynamically and generate a very complicated allowed list. Instead, we can add an allowedViolations property to the API and implement the same functionality with a few simple lines of code. My colleague implemented this API here: #285

The API to allow certain violations looks like

allowedViolations = [
    ":a:impl": [
        ":b:impl",
        ":c:impl"   
     ],
     ":b:impl": [
        ":c:impl"
    ]
]

@Laimiux Laimiux reopened this Oct 16, 2024
@jraska
Copy link
Owner

jraska commented Oct 26, 2024

Hey, I commented here.

At the end it seems just as a change from List<Entry<String, String>> into Entry<String, List<String>> do I get it correctly?

You can achieve the same Entry<String, List<String>> by regex "a:impl -> b:impl|c:impl|d:impl..." using existing API 🤔

@Laimiux
Copy link
Author

Laimiux commented Oct 28, 2024

The problem is that the library only matches on the alias and not the project path. Allowing impl -> :b:impl is too broad and if you use project path as alias, you have to rebuild all other rules a:impl -> b:impl and a:impl -> public etc. Why was regex chosen as the API? I wonder if a lambda (Project Path, Alias, Dependency) -> Boolean would be a more flexible API that enabled much more customization by the consumer. This would get rid of the need for the API to support separate allowed/rejected properties (and would enable us to define our rules in a simpler manner).

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

No branches or pull requests

2 participants