Output cross-language names in generated docs #130

RomainMuller · 2018-08-02T10:29:11Z

Each Target implementation can contribute to this process by providing
two static functions that allow the sphinx generator to obtain:

The package coordinates in the "standard" registry (e.g: maven) for an
assembly
The native name (or names) for a given Type. There can be multiple
names in case the artifacts can be used across different languages,
for example Javascript & TypeScript share names, but InterfaceTypes
are not visible in Javascript.

The C# names are not included, because the process to come up with those
names is factored in the C# code, and the dis-ambiguation is complex to
reproduce. This omission will be corrected at a later point in time.

Fixes #74

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Each `Target` implementation can contribute to this process by providing two static functions that allow the sphinx generator to obtain: * The package coordinates in the "standard" registry (e.g: maven) for an assembly * The native name (or names) for a given `Type`. There can be multiple names in case the artifacts can be used across different languages, for example Javascript & TypeScript share names, but `InterfaceType`s are not visible in Javascript. The C# names are not included, because the process to come up with those names is factored in the C# code, and the dis-ambiguation is complex to reproduce. This omission will be corrected at a later point in time. Fixes #74

eladb · 2018-08-02T10:40:05Z

packages/jsii-pacmak/bin/jsii-pacmak.ts

@@ -16,7 +16,7 @@ import { VERSION } from '../lib/version';
    const argv = yargs
        .usage('Usage: jsii-pacmak [-t target,...] [-o outdir] [package-dir]')
        .option('targets', {
-            alias: 't',
+            alias: ['target', 't'],


eladb · 2018-08-02T10:41:26Z

packages/jsii-pacmak/lib/target.ts

@@ -106,6 +107,21 @@ export abstract class Target {
 }

 export interface TargetConstructor {
+    /**
+     * Provides the coordinates of an assembly in the usual package repository for the target.


Explain what "coordinates" mean in this context. Maybe provide example(s)

eladb · 2018-08-02T10:41:41Z

packages/jsii-pacmak/lib/targets/java.ts

+            && (parsedVersion.major !== 0 ? parsedVersion.inc('major')
+                                          : parsedVersion.inc('minor'));
+        return {
+            repository: 'Maven',


Maven Central?

eladb · 2018-08-02T10:42:02Z

packages/jsii-pacmak/lib/target.ts

+     * @param assm the assembly for which coodinates are requested.
+     * @return the name of the repository, and the coordinates within it.
+     */
+    toPackageCoordinates?: (assm: spec.Assembly) => { repository: string, coordinates: string };


Should we add a url?

I thought so. But how to we format the link? I kinda punted this for later, but if you have an idea...

In the sphinx docs? Maybe something like:

<a href="link">View in ${repository}</a>

eladb · 2018-08-02T10:42:42Z

packages/jsii-pacmak/lib/targets/java.ts

+        };
+    }
+
+    public static toNativeNames(type: spec.Type, options: any) {


Reuse this method in java_generator.toNativeFqn

I wanted to, but the java generator's toNativeFqn only has a fqn(string) available, because it needs to also be able to render names for TypeRefs.

eladb · 2018-08-02T11:02:25Z

packages/jsii-pacmak/lib/targets/js.ts

+    public static toPackageCoordinates(assm: spec.Assembly) {
+        return {
+            repository: 'NPM',
+            coordinates: JSON.stringify({ [assm.name]: `^${assm.version}` }, null, 2)


Maybe we can also do something like:

npm i ${assm.name}@${assm.version}

Add another field to coordinates? (usage)

That's a good idea.

eladb · 2018-08-02T11:03:46Z

packages/jsii-pacmak/lib/targets/sphinx.ts

@@ -46,6 +47,11 @@ class SphinxDocsGenerator extends Generator {
        this.code.indentation = 3;
    }

+    public async load(packageRoot: string) {


I wonder if we shouldn't just include set of target constructors as part of load. It is available in the outer scope.

I guess we can couple everything to everything else. Doesn't mean it's a good idea...

Let's keep it like this for now until the 2nd case where a generator will need this info

eladb · 2018-08-02T11:04:36Z

packages/jsii-pacmak/test/expected.jsii-calc-base/sphinx/_scope_jsii-calc-base.rst

+
+   .. group-tab:: Maven
+
+      ::


Shouldn't that be .. code-block:: or something?

When using code-block, the "title" of the tab must be a valid syntax highlighter... And that's to the experience I want (would have Maven Central tab require to be named XML).

I think the titles should actually be the language name and not the repository name.

Also, you could have full control over the tab name if you just embed a code-block in it:

.. group-tab:: MyTitle .. code-block:: js console.log('hello, world')

Mind you, this will not render the same. But since we want to possibly also have a URL and "usage", this is probably something that's going to fly anyway :)

eladb · 2018-08-02T11:05:25Z

packages/jsii-pacmak/test/expected.jsii-calc-lib/sphinx/_scope_jsii-calc-lib.rst

@@ -1,6 +1,30 @@
 @scope/jsii-calc-lib
 ====================

+.. tabs::
+
+   .. group-tab:: Maven


The names of the group tabs should align with the names of the code tabs, so they will switch together

Can do, but I am not a fan of labelling the maven POM dependency blob with Java. In this case, what if we want to also provide a gradle instruction?

We can also provide gradle instructions (and probably also "yarn").

eladb · 2018-08-02T11:07:33Z

packages/jsii-pacmak/test/expected.jsii-calc-lib/sphinx/_scope_jsii-calc-lib.rst

+
+      .. code-tab:: javascript
+
+         Value


I think typescript and javascript also need the require statement:

JavaScript:

const { Value } = require('jsii-calc-lib');

TypeScript:

import { Value } from 'jsii-calc-lib'

Didn't we say we wanted to encourage people using import lib = require('lib');? This would go against this guideline...

It's true. But I can't think of a way to express this in a nicer way, and the name of the import is important here. Maybe:

import jsiiCalcLib = require('jsii-calc-lib'); jsiiCalcLib.Value

But that looks ugly.
I think import {} is fine here. The CDK examples and code should still use namespaced imports, but this is "The Right" way to express this in JS/TypeScript.

That's fair :)

eladb · 2018-08-02T13:19:17Z

Note that this will change the output directory to "js" instead of "npm", and will likely break our publishing scripts

RomainMuller · 2018-08-02T13:46:17Z

@eladb The name change is intentional - other folders are named after the language, not the repository.

eladb · 2018-08-02T13:48:20Z

@RomainMuller I am aware. Just calling it out. I think we'll not include this change in the release today anyway.

…how-names

RomainMuller · 2018-08-06T12:52:35Z

The rename was "copied" under this PR, so the output directory of javascript assets shouldn't be a concern any longer.

eladb

.NET is missing. Let's not leave it behind anymore...

eladb · 2018-08-06T12:53:25Z

packages/jsii-pacmak/lib/target.ts

+     *
+     * @return Information about the assembly in the various package managers supported for a given language.
+     */
+    toPackageInfos?: (assm: spec.Assembly) => { [language: string]: PackageInfo };


Provide some examples here. Specifically, it's not clear why would this return a hash instead of just PackageInfo (I understand this is for the typescript/javascript case, but maybe worth mentioning as an example)

Added a couple of examples.

eladb · 2018-08-06T12:54:03Z

packages/jsii-pacmak/lib/target.ts

+     *
+     * @return the native reference for the target for each supported language.
+     */
+    toNativeReference?: (type: spec.Type, options: any) => { [language: string]: string };


eladb · 2018-08-06T12:54:13Z

packages/jsii-pacmak/lib/target.ts

+     * plain string (documentation should render as a pre-formatted block of text using monospace font), or an object
+     * describing a language-tagged block of code.
+     */
+    usage: { [label: string]: string | { language: string, code: string } };


eladb · 2018-08-06T12:56:45Z

packages/jsii-pacmak/lib/targets/sphinx.ts

+ */
+function formatLanguage(language: string): string {
+    switch (language) {
+    case 'csharp':


Haven't seen the .NET target changes. Should we at least emit a C# option there?

Since we're not publishing anything to Nuget yet, I figured it was probably best if our documentation doesn't start pointing people at places that don't exist...

#149 created to track the C# fragment.

RomainMuller requested review from rix0rrr and eladb August 2, 2018 10:29

eladb reviewed Aug 2, 2018

View reviewed changes

RomainMuller added 6 commits August 6, 2018 14:18

Some PR feedback

a48e5f7

Merge remote-tracking branch 'origin/master' into rmuller/74-sphinx-s…

d6eec12

…how-names

Correct capitalization of tab names & sorted tabs uniformly.

07a3562

Fix expectations (minimal)

039c352

Merge remote-tracking branch 'origin/master' into rmuller/74-sphinx-s…

677e82b

…how-names

Fix expectations (final?)

6aecac8

eladb suggested changes Aug 6, 2018

View reviewed changes

Added some example in comments

c0ef739

RomainMuller mentioned this pull request Aug 6, 2018

Generate DotNet (C#) names & package info in documentations #149

Closed

eladb approved these changes Aug 6, 2018

View reviewed changes

RomainMuller merged commit e9c6a06 into master Aug 6, 2018

RomainMuller deleted the rmuller/74-sphinx-show-names branch August 6, 2018 13:25

Output cross-language names in generated docs #130

Output cross-language names in generated docs #130

Conversation

RomainMuller commented Aug 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eladb commented Aug 2, 2018

RomainMuller commented Aug 2, 2018

eladb commented Aug 2, 2018

RomainMuller commented Aug 6, 2018

eladb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment