y-lohse / inkjs

A javascript port of inkle's ink scripting language.
http://www.inklestudios.com/ink/
MIT License
506 stars 103 forks source link

Proposing JSDocs Documentation #524

Closed videlais closed 11 months ago

videlais commented 4 years ago

Describe the bug

I'd really love for there to be documentation of InkJS, so I'm volunteering to port over the existing C# documentation using JSDocs-supported types.

(I also noticed that the JSDocs plugin was installed for ESLint in the eslint branch, so I assume this is at least being considered?)

Additional context

Here's some initial work on Choice using the C# descriptions to show an example of what it might look like.

Note: 'eslint -- fix' was run on this before I started with JSDocs, so a few things were changed by that process.

/**
 * A generated Choice from the story.
 *
 * A single ChoicePoint in the Story could potentially generate different Choices dynamically dependent on state, so they're separated.
 *
 * @export
 * @class Choice
 * @extends InkObject
 */
export class Choice extends InkObject{
    // The main text to presented to the player for this Choice.
    public text = '';
    // The original index into currentChoices list on the Story when this Choice was generated, for convenience.
    public index = 0;
    public threadAtGeneration: CallStack.Thread | null = null;
    // Get the path to the original choice point - where was this choice defined in the story?
    public sourcePath = '';
    public targetPath: Path | null = null;
    public isInvisibleDefault = false;
    public originalThreadIndex = 0;

    /**
     * pathStringOnChoice - The target path that the Story should be diverted to if this Choice is chosen.
     *
     * @type {string}
     * @memberof Choice
     */
    get pathStringOnChoice(): string{
        if (this.targetPath === null) return throwNullException('Choice.targetPath');
        return this.targetPath.toString();
    }
    /**
     * pathStringOnChoice - The target path that the Story should be diverted to if this Choice is chosen.
     *
     * @param {string}
     * @memberof Choice
     */
    set pathStringOnChoice(value: string){
        this.targetPath = new Path(value);
    }
}
y-lohse commented 4 years ago

Hi! My first question is… why? 🙃

To give you some context, the current policy is to remove all comments from the original code, and add comments for sections that differ significantly from the C# source. The rationale behind that is:

  1. The C# repo and code is the reference place to learn about ink, including how the engine works under the hood
  2. Less to maintain (changes in comments upstream would need to be updated here as well)
  3. No confusion between what is specific to inkjs

If we only include the C# doc comments from upstream as you suggest, and omit any other inline comment, then I think 3 wouldn't be an issue. Maintaining the comments would still be an extra thing to do, but I don't think they change a whole lot, so it's not out of the question.

That leaves me with the question of why: why would inkjs benefit from having these comments in the source, and is it worth the extra maintenance and initial effort on your end?

videlais commented 4 years ago

Fair question.

Part of the problem is an implicit assumption in your answer: “the C# repo and code is the reference place to learn about ink.” What if you want to learn about Ink, and InkJS in particular, without digging through the C# code? There is no external documentation for the C# code, nor an easy way to learn it without already knowing enough to dig through it. And then someone would also have to dig through and test things.

(Along with writing the Unofficial Ink Cookbook, I am also writing a book project on Ink + Unity that discusses C# code to help solve this issue.)

Parallel to this issue is another that InkJS is used with the “Ink for Web” option in Inky, but does not fully explain why its own code uses certain methods. At least currently, it can’t do that, even if it wanted to, because there is no documentation on this side to point to from it.

I should have specified “in addition to existing comments” in my proposal. I don’t mean to lose the existing comments! But if the only criteria is where they are notably different, more work is needed there as well. For example, the ObserveVariable() method is different in InkJS than the C# code, but is not mentioned in the README.

If the answer is “This is extra work and we do not see a strong reason to do it right now”: totally understandable. I can personally see potential use cases where people could learn to work with Ink using just InkJS (and have, in fact, written guides for just that) and could even see a future use case where the TypeScript JSDocs was included in an NPM build where programs like Visual Studio Code could read it and provide developers with method definitions as they work.

In my best-case scenario, the documentation could be merged between the existing C# comments and the existing ones. From there, a tool could be used to pull it out and present it in a wiki or other format for more easily reading it. This is an additional step from the documentation itself! And more work! But, it would allow a new user to learn the code without needing to dig through the TypeScript or, even worse, digging through TypeScript AND C# code to figure something out.

To put this is summary: I’d like to better learn, and teach, InkJS without needing to read through the TypeScript AND C# code. Trying to be fluent in two other languages (TypeScript and C#) to use a third (JavaScript) is a large cognitive ask of people. But I also fully understand that it is some amount of hours of initial work to get started and also an unknown amount in the future to maintain.

On May 26, 2020, at 4:32 AM, Yannick Lohse notifications@github.com wrote:

 Hi! My first question is… why? 🙃

To give you some context, the current policy is to remove all comments from the original code, and add comments for sections that differ significantly from the C# source. The rationale behind that is:

The C# repo and code is the reference place to learn about ink, including how the engine works under the hood Less to maintain (changes in comments upstream would need to be updated here as well) No confusion between what is specific to inkjs If we only include the C# doc comments from upstream as you suggest, and omit any other inline comment, then I think 3 wouldn't be an issue. Maintaining the comments would still be an extra thing to do, but I don't think they change a whole lot, so it's not out of the question.

That leaves me with the question of why: why would inkjs benefit from having these comments in the source, and is it worth the extra maintenance and initial effort on your end?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

y-lohse commented 4 years ago

I’d like to better learn, and teach, InkJS without needing to read through the TypeScript AND C# code.

Ok, I think that's fair and I probably would have enjoyed that myself :)

But I also fully understand that it is some amount of hours of initial work to get started and also an unknown amount in the future to maintain.

I'm not too worried about the maintenance, the Ink API is very stable and although I haven't really looked at comment changes over time, I don't think there were too many.
And the initial work… Well if you take care of it, I don't see a problem 😁

@ephread what do you think?

ahayworth commented 4 years ago

I'd like to voice support for some kind of documentation, somewhere. 😄

I've dug around in the guts of both inkjs and upstream C# ink - and while I'm comfortable doing such a thing, having docs on the internals of one or the other would have been a great help. In the absence of docs on the C# upstream project, I feel that docs here would be valuable.

Of course, I'm also not the one volunteering to do the work, so take my support with a grain of salt. ❤️ I wouldn't want docs to become a maintenance burden, if there's not a huge need for them.

videlais commented 4 years ago

I primarily use Ink either in Inky (for teaching) or via InkJS for my own projects. It did not occur to me till right now, but would it make more sense to ask about commenting the upstream code more and then "inheriting" those comments as translated to TypeScript JSDocs later?

The initial work would be be roughly the same to add in more XML comments, I'd think. And there are also programs out there for converting the XML comments into JSDocs format.

ephread commented 4 years ago

Lots of good arguments in this thread!

@y-lohse seeing comments in a codebase triggers happiness in my primitive brain, so I'm totally in favour of having code documentation in inkjs.

There's a major downside with JSDoc in TypeScript though: the type information is duplicated.

For that reason, I'd be inclined to skip adding information that can be inferred, but at the same time, I don't think there's any way to parse comments and insert type information in the compiled JavaScript. If someone knows a way, let me know!

That means the comments will invariably get out of sync with the codebase. But as long as @videlais or someone else is willing to keep an eye on them as the code changes and open PRs to fix oversights, I'm okay with having them in inkjs.


It did not occur to me till right now, but would it make more sense to ask about commenting the upstream code more and then "inheriting" those comments as translated to TypeScript JSDocs later?

I guess both codebase would benefit from having more documentation anyway. Having documentation hints in IntelliSense (JavaScript side) would probably be quite nice!

y-lohse commented 4 years ago

It did not occur to me till right now, but would it make more sense to ask about commenting the upstream code more and then "inheriting" those comments as translated to TypeScript JSDocs later?

The "inheritance" would still be a lot of manual labour I guess, so if your end goal is to have more docs in the js engine I'm not sure it's necessary worth it tbh. But worth exploring :)

For that reason, I'd be inclined to skip adding information that can be inferred, but at the same time, I don't think there's any way to parse comments and insert type information in the compiled JavaScript. If someone knows a way, let me know!

If the end goal is to export the docs into a wiki o an explorable interface or something, maybe we could use plugins like this one to reduce the amount of duplicated informations without sacrificing the output? Ideally all we would add is plain english descriptions, right?