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

Issues#238 Rule About package name #246

Merged
merged 4 commits into from
Jul 24, 2018
Merged

Conversation

yokotaso
Copy link
Contributor

@yokotaso yokotaso commented Jul 1, 2018

Hi, Thank you for review #228 #243

And I develop #238

And then, I noticed bellow coding style rules. This PR includes addtional rule.

Thanks!

- package name should match directory name which kotlin file exists.
- package name should not contians UpperCase characters and UnderScore.
  https://android.github.io/kotlin-guides/style.html#package-names
@yokotaso
Copy link
Contributor Author

yokotaso commented Jul 5, 2018

In this rule, We can use camel case package name.
https://kotlinlang.org/docs/reference/coding-conventions.html#naming-rules

@shyiko
Copy link
Collaborator

shyiko commented Jul 24, 2018

Thank you, @yokotaso!

@shyiko shyiko merged commit 1ce58b3 into pinterest:master Jul 24, 2018
@yokotaso yokotaso deleted the Issue-238 branch July 25, 2018 06:24
@thauk-copperleaf
Copy link

@yokotaso @shyiko This change does not check for UpperCase characters in package names; in fact, it now has a test to make sure that they are allowed:
https://github.com/shyiko/ktlint/pull/246/files#diff-33c3b4f79a7f6e0127d5a5efc15fee3fR46

@yokotaso
Copy link
Contributor Author

@thauk-copperleaf yes. UpperCase is allwoed.
Because of this rule : https://kotlinlang.org/docs/reference/coding-conventions.html#naming-rules

But UpperCase is now allowed this rule:
https://android.github.io/kotlin-guides/style.html#package-names

@thauk-copperleaf
Copy link

@yokotaso The first link disallows them, then allows them in the same paragraph. Very confusing!

The second link disallows them.

// WRONG!
package com.example.deepSpace

@shyiko
Copy link
Collaborator

shyiko commented Jul 27, 2018

While I'm 100% with Android Style Guide on this (keep package names in lowercase), ktlint is not enforcing this rule for one simple reason: there are plenty of projects that use ktlint and have package names in camelCase. We cannot expect people to rename packages considering a breaking nature of the change. Hence - not enforced.

@sanogueralorenzo
Copy link

sanogueralorenzo commented Aug 18, 2018

@shyiko Many projects also have package names such as: big_example and introducing any rule regarding this stops these big sized projects from updating their ktlint version.

Do you have any suggestions for these kind of projects?

@shyiko
Copy link
Collaborator

shyiko commented Aug 18, 2018

@sanogueralorenzo right now the only option is to use ktlint-disable, e.g.

package x_y // ktlint-disable package-name

/* ktlint-disable package-name */
package x_y

While I always check a handful of public repos before adding new rules (there are no packages containing _ in Mozilla/Google/WordPress/... repos) I realize that some of the assumptions might hold true for everyone. If you can provide a list of projects that have _ in their package names - I'll consider removing the rule from ktlint.

@thauk-copperleaf
Copy link

@sanogueralorenzo Isn't that possible for any rule ktlint may introduce?

@shyiko
Copy link
Collaborator

shyiko commented Aug 18, 2018

@thauk-copperleaf not really.

Let's assume project foo (used by baz, qux, etc) has a package com.foo.bar_baz. You are the maintainer of foo. Imagine you decided to add ktlint. Differences in code style can be easily rectified (using ktlint --format or manually) but not the package name (as all the projects that depend on foo (baz, qux, etc) would have to be updated to use com.foo.barbaz instead of com.foo.bar_baz).

@thauk-copperleaf
Copy link

What I mean is, ktlint could introduce a rule that is violated by a major software project, so is that a sufficient reason to not introduce the rule?

@shyiko
Copy link
Collaborator

shyiko commented Aug 19, 2018

For a rule to be added

  • it must not force people to introduce backward-incompatible changes
    (renaming packages, classes, etc are all breaking changes).

One way to deal with this situation is to keep all (potentially) breaking rules enabled by default but disable them when --strict=false. The other - --disable-<rule_id>...

  • it should (preferably) be based on the official recommendations.

Whether it's violated by "a major software project" does not matter.

@sanogueralorenzo
Copy link

Did further research and package name with underscore is accepted but never recommended. While it is true that most repos don't follow this rule it still leaves some projects that might have it because for reason X decided to not follow this convention. 😅

Comparing it to the wildcard import (that could easily get fixed with the help of the IDE and format) this one is a bit trickier as seen in the example above by @shyiko

Current solutions that I can think of:

  1. Something like (rule):allow wildcard imports via editorconfig #61 but I wouldn't like this idea since it would require branching off & keeping it up to date.
  2. Stay in 0.24.0 since it doesn't contain the package-name rule.
  3. Introducing something like the --disable so it is ignored (this would open the possibility of letting the developer ignore any of the rules?)
  4. Slowly refactor package naming incrementally.

Thoughts?

@yokotaso
Copy link
Contributor Author

@shyiko @sanogueralorenzo

Sorry for my poor implementation rule.
I will resend PR which remove this rule about package name and checks.
Only check whether directory name and package name is correct.

yokotaso added a commit to yokotaso/ktlint that referenced this pull request Aug 21, 2018
@yokotaso
Copy link
Contributor Author

yokotaso commented Aug 21, 2018

@shyiko
Thank you for maintenance ktlint!
I have read this thread and then rename PackageNameRule to DirectoryStructureRule.
DirectoryStructureRule will only check https://kotlinlang.org/docs/reference/coding-conventions.html#directory-structure

I'm sorry to trouble apologize... 😭

@shyiko
Copy link
Collaborator

shyiko commented Aug 21, 2018

@yokotaso you have absolutely nothing to be sorry about. The rule works the way it was designed. Honestly, you did a good job.

I'm still not sure we should disable it (// ktlint-disable package-name works after all).

@sanogueralorenzo
Copy link

@yokotaso Not at all! Sorry if my message came across that way. I was just asking for suggestions to deal with a situation where a project is using _ extensively due to a previous convention.

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.

None yet

4 participants