-
Notifications
You must be signed in to change notification settings - Fork 26
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 remaining metadata #449
Conversation
Codecov Report
@@ Coverage Diff @@
## master #449 +/- ##
=======================================
Coverage 66.28% 66.28%
=======================================
Files 67 67
Lines 1815 1815
Branches 281 281
=======================================
Hits 1203 1203
Misses 489 489
Partials 123 123 Continue to review full report at Codecov.
|
I don't know enough about this api stuff, perhaps someone on the intellij team who might be using it can review? |
"mavenCoordinates" : { | ||
"groupId" : "com.google.cloud", | ||
"artifactId" : "google-cloud-resourcemanager", | ||
"version" : "0.23.0-alpha" |
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.
Should this be versions? how do we encode the BOM data?
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.
These entries should be defined in relation to a common BOM: we don't want to mix-and-match between BOMs, right? The BOM definition could be be external (e.g., encoded in the file-name like libraries-0.22.json
), or we could make that an outer element if you wanted a single library definition.
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.
We could pull this from the google-cloud-java pom.xml though it's an extra lookup.
E.g. https://search.maven.org/remotecontent?filepath=com/google/cloud/google-cloud-pom/0.23.1-alpha/google-cloud-pom-0.23.1-alpha.pom
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.
It would be worth having a test that pulls the dependencies for each and ensures there's no conflicting versions.
"apireference":"https://googlecloudplatform.github.io/google-cloud-java/latest/apidocs/index.html?com/google/cloud/dlp/v2beta1/package-summary.html", | ||
"infotip":"The com.google.cloud.dlp.v2beta1 packages", | ||
"launchStage" : "alpha", | ||
"packages" : ["com.google.cloud.dlp.v2beta1"], |
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.
what does packages represent?
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 Java packages exported by the library. We can use them in quick-fixes to add a library on an unresolved symbol.
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.
We have to manually manage this though. Couldn't you just build a package index from the library?
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.
That would require downloading each of the libraries (and possibly dependencies) and process them. And it also doesn't help us avoid exposing shaded jars (e.g., *.repackaged.*
).
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.
Aha I see how this can be useful, but I'm still skeptical of our ability to maintain this data. Until we have concrete plans to use this data I'd prefer if we left it out.
I really like the idea of suggesting libraries based off of usage, but I'd like that feature designed first.
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.
This is already a feature available for plugin development in Eclipse, where Eclipse PDE scans for the packages and classes offered by the available plugins and offers to configure the plugin classpath on an unresolved symbol:
That said, the real help is PDE recognizing class names, not package names. E.g., that BigtableDataClient
→ com.google.cloud.bigtable
. So +1 to removing the package names.
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.
@briandealwis thanks for the details and screenshot!
Ping |
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 only nit is that the spacing before and after ':' in the JSON is inconsistent.
"mavenCoordinates" : { | ||
"groupId" : "com.google.cloud", | ||
"artifactId" : "google-cloud-resourcemanager", | ||
"version" : "0.23.0-alpha" |
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.
These entries should be defined in relation to a common BOM: we don't want to mix-and-match between BOMs, right? The BOM definition could be be external (e.g., encoded in the file-name like libraries-0.22.json
), or we could make that an outer element if you wanted a single library definition.
I'm checking with the internal Javascript stylists to see if they prefer "foo":"bar" or "foo" : "bar". |
The PRD specifies that the selector should also include a link to the issue tracker for each library. Should we add this? |
What's the purpose of the issue tracker link? Is this something we want to expose in the UI? |
For making requests for changes or enhancements to the APIs? Saves the developer from having to hunt the right tracker down, and becoming confused by the com.google.api.* vs com.google.cloud.* repositories. |
I'm very skeptical of exposing the API issue trackers in the IDEs. This is unlikely to appear at a place where the user might want it (on the code). It's more likely to collect random bugs that have nothing to do with the APIs or CT4E. |
I think its convenient (and can't hurt) to have this link. At a minimum, it gives the user the ability to quickly view open issues & activity prior to adding the library. I do this sometimes myself. However I'll defer to the PRD authors. @patflynn wdyt? |
It's easier to add something later than take it out, so I default to leaving fields out until we have consensus that they're needed. Also note that for these libraries there is exactly one issue tracker for all 23 APIs, not separate issue trackers for each. |
I think it's something we want eventually but I'm fine leaving it out for now. Like Elliotte said, for this first batch of libraries they're all in the same github tracker and the phase 0 UX would probably only display the tracker info way before the user has had a chance to find any issues. |
* add translate metadata * stackdriver logging * pubsub * spanner * vision * languageLevel * better names * compute * 0.23.0 * 1.5.0 * data loss prevention * cloud DNS * error reporting * video intelligence * stackdriver trace * cloud speech * fix logo URLs * resource manager * monitoring * dlp URLs * assorted fixes * 1.5.1 * remove packages field
* Dependency version bumps: - Bump commons-io from 2.4 to 2.11.0 - Bump junit from 4.12 to 4.13 - Bump hamcrest-library from 1.3 to 2.2 - Bump mockito-core from 2.11.0 to 4.11.0 - Bump jacoco from 0.8.6 to 0.8.8 - Bump checkstyle from 8.18 to 8.37 * Have `verifyGoogleFormat` task run before checkstyle tasks (to suggest fixing via `./gradlew googleJavaFormat`)
@briandealwis @loosebazooka @patflynn @etanshaul got a bit confused by the capitalization, but this is the current branch