-
Notifications
You must be signed in to change notification settings - Fork 76
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
JLBP: Declare all dependencies #1767
Changes from 3 commits
246b15d
950379e
af8178f
665e7d9
6d9e34e
a6b9969
4e19a5a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,56 @@ | ||||||
--- | ||||||
jlbp: | ||||||
id: JLBP-22 | ||||||
permalink: /JLBP-22 | ||||||
--- | ||||||
|
||||||
# Declare all direct dependencies | ||||||
|
||||||
If your code references a class—for example, by invoking a method in that class—declare a dependency that includes that class in your pom.xml, build.gradle, | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a formatting practice for referring to files? Should this be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we've chosen one yet. Might be worth checking general google style and general cleanup. |
||||||
or equivalent. In Bazel this practice is called "strict deps". | ||||||
|
||||||
Code should not call methods, reference fields, or instantiate classes from _indirect_ dependencies. These are dependencies of the declared dependencies. Projects that rely on indirect dependencies tend to break in unexpected ways when direct dependencies are upgraded. | ||||||
suztomo marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
||||||
For example, your project might declare a dependency on the | ||||||
Google HTTP Java Client which | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is a restrictive clause, but it's arguable. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If it were restrictive, that would mean that there are several Google HTTP Java Clients, only one of which depends on Apache HTTP Components. Is that the case? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's one way of identifying restrictive subordinate clauses, but a little more generally, "A restrictive adjective clause, on the other hand, is essential to a sentence and should not be set off by commas" https://www.thoughtco.com/restrictive-and-nonrestrictive-adjective-clauses-1689689 I think this clause is indeed essential to the sentence. A better reference than that will have to wait until I get back to the office to retrieve my Chicago Manual of Style. :-) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The content of the clause is important to the meaning of the paragraph, but not essential to the sentence. "For example, your project might declare a dependency on the Google HTTP Java Client." That doesn't change the reader's understanding of what you mean by "the Google HTTP Java Client", although it leaves out an important detail about why you're talking about it. Compare to your sentence above: "Projects that rely on indirect dependencies have an annoying habit of breaking in unexpected ways when direct dependencies are upgraded." If you got rid of the restrictive clause and changed it to "Projects have an annoying habit of breaking in unexpected ways when direct dependencies are upgraded", the reader wouldn't know which projects you were talking about. Anyway, it's your document. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it needs a comma for the reason David explained:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The point is "essential to a sentence," which clause this is. It is not whether it is essential to the meaning of the modified noun or noun phrase. |
||||||
itself depends on the Apache HTTP Components. If so, it is possible to use the `org.apache.http.client.utils.URLEncodedUtils` class in your own project without | ||||||
explicitly declaring a dependency on Apache HTTP Components. However, you should | ||||||
add the dependency anyway. This way if a future version of the | ||||||
suztomo marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
Google HTTP Java Client no longer depends on Apache HTTP Components, your code | ||||||
still compiles. Strict dependencies also help static analysis tools better understand a project. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would you add names of these static analysis tools you have in mind? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not needed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I'm asked from somebody about this static analysis tools, I cannot answer it. Would you help me in this regard? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "Strict deps" appears in a previous paragraph, but 'Strict dependencies" does not. Would you pick one? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. bazel does call it "strict deps" bit otherwise we try not to abbreviate There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bazel call it in that way, but this document does not name this practice. |
||||||
|
||||||
This doesn't only happen when a project itself is upgraded. It can also | ||||||
suztomo marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
happen when dependency mediation selects a different version of a library's dependency | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe give examples of when dependency mediation might change versions other than when you change your declared dependency versions. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm hesitant to have a single sentence paragraph. |
||||||
that does not include the necessary indirect dependency. Relying on indirect dependencies can cause problems for your customers that you don't see. | ||||||
suztomo marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
||||||
IDE autocomplete suggestions are a common way projects come to depend on | ||||||
indirect dependencies. When importing a new class, most IDEs only look to see if it's present in the classpath, not whether it comes from a direct or indirect dependency. | ||||||
|
||||||
The `mvn dependency:analyze` command lists dependencies a Maven project uses | ||||||
but hasn't declared: | ||||||
|
||||||
``` | ||||||
[WARNING] Used undeclared dependencies found: | ||||||
[WARNING] org.apache.maven.resolver:maven-resolver-impl:jar:1.4.1:compile | ||||||
[WARNING] org.apache.maven.resolver:maven-resolver-api:jar:1.6.1:compile | ||||||
[WARNING] org.apache.maven:maven-core:jar:3.6.3:compile | ||||||
[WARNING] org.apache.maven:maven-model-builder:jar:3.6.3:compile | ||||||
``` | ||||||
|
||||||
These should be added to your pom.xml file. | ||||||
|
||||||
The tool also lists dependencies the project declares but doesn't use: | ||||||
suztomo marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
||||||
``` | ||||||
[WARNING] Unused declared dependencies found: | ||||||
[WARNING] com.google.cloud.tools:dependencies:jar:1.5.5-SNAPSHOT:compile | ||||||
[WARNING] com.google.truth:truth:jar:1.0.1:test | ||||||
[WARNING] junit:junit:jar:4.13.1:test | ||||||
[WARNING] org.mockito:mockito-core:jar:3.5.15:test | ||||||
``` | ||||||
|
||||||
However its analysis of which dependencies aren't used is not as accurate | ||||||
as its analysis of which dependencies are used. In particular, | ||||||
it reports dependencies used through reflection as unused, so be cautious when | ||||||
removing any allegedly unused dependencies. | ||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The varying line lengths make this hard to review. Can you run mdformat?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-bash: mdformat: command not found