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

Add filtering option for excluding specific configuration name #25

Conversation

omuomugin
Copy link
Contributor

πŸš€ Description

Try to add excludeConfigurationNames option which you can ignore specific configuration names.

Fix #21

example usage is below

moduleGraphConfig {
    heading.set("# Module Graph")
    readmePath.set("./README.md")

    // optional configs
    excludeConfigurationNames.set(listOf("testImplementation"))
}

πŸ“„ Motivation and Context

I have same situation as #21 .
I know @iurysza is writing POC with Regex seeing https://github.com/iurysza/module-graph/tree/regex-filter .
However as #21 stated, I think there is a major case that want to ignore specific configuration (especially testImplementation ) for document use.

I'm open to discuss about this feature so just let me know your opinion.

πŸ§ͺ How Has This Been Tested?

Added some test cases for ModuleGraphPluginFunctionalTest
And also tested with sample project.

πŸ“¦ Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

βœ… Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

@omuomugin omuomugin mentioned this pull request Feb 6, 2024
@@ -7,4 +7,6 @@ dependencies {
implementation(project(":sample:beta"))
implementation(project(":sample:container:gama"))
implementation(project(":sample:container:delta"))

testImplementation(project(":sample:test"))
Copy link
Contributor Author

@omuomugin omuomugin Feb 6, 2024

Choose a reason for hiding this comment

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

Added for checking ignoring option.
I executed command below and checked README.md hasn't changed.

./gradlew :sample:createModuleGraph

@omuomugin
Copy link
Contributor Author

@iurysza Is there any chance to get reviewed ??

@iurysza
Copy link
Owner

iurysza commented Mar 22, 2024

Hey @omuomugin, thanks for the PR.
Sorry for the delay, I just noticed this now. I'll check it out over the weekend.

@omuomugin
Copy link
Contributor Author

@iurysza Have you got time for review? I'm open for discussion, so let me know your thoughts.

README.md Outdated
@@ -166,6 +166,8 @@ Optional settings:
- `NONE`: No text added. (Default.)
- `CONFIGURATION`: The name of the configuration which the dependency belongs to (e.g. "
implementation", "compileOnly", "jsMain").
- **excludeConfigurationNames**:
- List of configuration name which should be ignored. e.g. "implementation", "testImplementation". Default is emptyList().
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
- List of configuration name which should be ignored. e.g. "implementation", "testImplementation". Default is emptyList().
- List of configurations to be ignored. e.g. "implementation", "testImplementation". Default is emptyList().

Copy link
Contributor Author

@omuomugin omuomugin Mar 27, 2024

Choose a reason for hiding this comment

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

As #25 (comment), #25 (comment) says
Maybe it should be "List of configuration names to be ignored" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed 73b5e93

@@ -47,6 +48,11 @@ abstract class CreateModuleGraphTask : DefaultTask() {
@get:Optional
abstract val linkText: Property<LinkText>

@get:Input
@get:Option(option = "excludeConfigurationNames", description = "List of configuration name which should be ignored")
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
@get:Option(option = "excludeConfigurationNames", description = "List of configuration name which should be ignored")
@get:Option(option = "excludeConfigurationNames", description = "List of configurations names to be ignored")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed 73b5e93

@@ -44,6 +45,13 @@ open class ModuleGraphExtension @Inject constructor(project: Project) {
*/
val linkText: Property<LinkText> = objects.property(LinkText::class.java)

/**
* List of configuration name which should be ignored.
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
* List of configuration name which should be ignored.
* Configuration names to be ignored.

Copy link
Contributor Author

@omuomugin omuomugin Mar 27, 2024

Choose a reason for hiding this comment

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

As #25 (comment), #25 (comment) says,
Maybe it should be "List of configurations to be ignored" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed 73b5e93

Copy link
Owner

@iurysza iurysza left a comment

Choose a reason for hiding this comment

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

Hey @omuomugin , this looks fine by me. Just some wording nitpicks.
Cheers

@@ -166,6 +166,8 @@ Optional settings:
- `NONE`: No text added. (Default.)
- `CONFIGURATION`: The name of the configuration which the dependency belongs to (e.g. "
implementation", "compileOnly", "jsMain").
- **excludeConfigurationNames**:
Copy link
Owner

Choose a reason for hiding this comment

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

Please add an example of usage in the code blocks above. Like here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added 2587876

@omuomugin
Copy link
Contributor Author

omuomugin commented Mar 27, 2024

@iurysza
Thank you for reviews.
I've fixed it.

Please re-review when you have time.

I'll be fixing up these additional commits when review is done.

73b5e93
2587876

@iurysza iurysza merged commit 327280c into iurysza:main Mar 28, 2024
1 of 4 checks passed
@iurysza
Copy link
Owner

iurysza commented Mar 28, 2024

This will be part of the v0.6.0 release.

@omuomugin
Copy link
Contributor Author

omuomugin commented Mar 28, 2024

@iurysza
Thanks for taking time!
Looking forward to be released.

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.

Ignore certain modules
2 participants