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

Enable lint on test sources by default #436

Merged
merged 1 commit into from
Jun 30, 2023
Merged

Conversation

ZacSweers
Copy link
Collaborator

Apparently this is not enabled by default 🤦‍♂️

Apparently this is not enabled by default 🤦‍♂️
@ZacSweers ZacSweers requested a review from valeraz June 30, 2023 21:58
@github-actions
Copy link

Here are some suggestions to improve the code:

  1. Simplify the SlackProperties class by using Kotlin's property delegation. This will eliminate the need for individual getter methods for each property. For example:
public class SlackProperties(private val project: Project) {
  public val lintBaselineFileName: String by stringProperty("slack.lint.baseline-file-name", "lint-baseline.xml")
  public val lintCheckTestSources: Boolean by booleanProperty("sgp.lint.checkTestSources", true)
  public val lintIgnoreTestSources: Boolean by booleanProperty("sgp.lint.ignoreTestSources", false)
  // ...
}
  1. In the LintTasks object, instead of assigning properties one by one, you can use the apply function to set multiple properties at once. For example:
internal object LintTasks {
  // ...

  val lintOptions = LintOptions().apply {
    // ...
    ignoreTestSources = slackProperties.lintIgnoreTestSources
    checkTestSources = slackProperties.lintCheckTestSources
    // ...
  }

  // ...
}
  1. Consider using a more descriptive name for the lintCheckTestSources and lintIgnoreTestSources properties in SlackProperties. The current names are a bit unclear and may cause confusion.

  2. Add comments to explain the purpose and usage of the lintCheckTestSources and lintIgnoreTestSources properties in SlackProperties. This will make it easier for other developers to understand their purpose.

  3. Consider using constants or enums instead of hardcoding string values for property keys. This will make the code more maintainable and less error-prone. For example:

private const val LINT_BASELINE_FILE_NAME_KEY = "slack.lint.baseline-file-name"
private const val LINT_CHECK_TEST_SOURCES_KEY = "sgp.lint.checkTestSources"
private const val LINT_IGNORE_TEST_SOURCES_KEY = "sgp.lint.ignoreTestSources"

public class SlackProperties(private val project: Project) {
  public val lintBaselineFileName: String by stringProperty(LINT_BASELINE_FILE_NAME_KEY, "lint-baseline.xml")
  public val lintCheckTestSources: Boolean by booleanProperty(LINT_CHECK_TEST_SOURCES_KEY, true)
  public val lintIgnoreTestSources: Boolean by booleanProperty(LINT_IGNORE_TEST_SOURCES_KEY, false)
  // ...
}

Overall, these suggestions aim to simplify and improve the readability of the code by leveraging Kotlin's language features and conventions.

@ZacSweers ZacSweers changed the title Enable lint onf test sources by default Enable lint on test sources by default Jun 30, 2023
@ZacSweers ZacSweers requested a review from arpita184 June 30, 2023 22:10
@ZacSweers ZacSweers added this pull request to the merge queue Jun 30, 2023
Merged via the queue into main with commit d2bb876 Jun 30, 2023
3 checks passed
@ZacSweers ZacSweers deleted the z/fixLintNotCheckingTests branch June 30, 2023 23:26
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