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

[api-documenter] Overloaded namespace-level functions named with '_' in YAML #1052

Closed
AlexJerabek opened this issue Jan 24, 2019 · 4 comments · Fixed by #1450
Closed

[api-documenter] Overloaded namespace-level functions named with '_' in YAML #1052

AlexJerabek opened this issue Jan 24, 2019 · 4 comments · Fixed by #1450
Labels
ae-ambiguous-ids This issue belongs to a set of bugs tracked by Pete bug Something isn't working as intended effort: easy Probably a quick fix. Want to contribute? :-) needs design The next step is for someone to propose the details of an approach for solving the problem

Comments

@AlexJerabek
Copy link
Contributor

In testing out v7 of API Extractor/Documenter, I found that overloaded namespace-level functions include an underscore in both their uid and name (as shown in the yaml sample below. Other overloaded functions do not have the underscore in their name.

  - uid: onenote.OneNote.run
    summary: >-
      Executes a batch script that performs actions on the OneNote object model, using a new request context. When the
      promise is resolved, any tracked objects that were automatically allocated during execution will be released.
    name: OneNote.run
    fullName: OneNote.run
[…]
  - uid: onenote.OneNote.run_1
    summary: >-
      Executes a batch script that performs actions on the OneNote object model, using the request context of a
      previously-created API object.
    name: OneNote.run_1
    fullName: OneNote.run_1

overloads

Sample d.ts:

export declare namespace OneNote {
    /**
     * Executes a batch script that performs actions on the OneNote object model, using a new request context. When the promise is resolved, any tracked objects that were automatically allocated during execution will be released.
     * @param batch - A function that takes in an OneNote.RequestContext and returns a promise (typically, just the result of "context.sync()"). The context parameter facilitates requests to the OneNote application. Since the Office add-in and the OneNote application run in two different processes, the request context is required to get access to the OneNote object model from the add-in.
     */
    export function run<T>(batch: (context: OneNote.RequestContext) => Promise<T>): Promise<T>;
    /**
     * Executes a batch script that performs actions on the OneNote object model, using the request context of a previously-created API object.
     * @param object - A previously-created API object. The batch will use the same request context as the passed-in object, which means that any changes applied to the object will be picked up by "context.sync()".
     * @param batch - A function that takes in an OneNote.RequestContext and returns a promise (typically, just the result of "context.sync()"). When the promise is resolved, any tracked objects that were automatically allocated during execution will be released.
     */
    export function run<T>(object: OfficeExtension.ClientObject, batch: (context: OneNote.RequestContext) => Promise<T>): Promise<T>;
}
@octogonz octogonz added the bug Something isn't working as intended label Jan 27, 2019
@octogonz
Copy link
Collaborator

This is a bug. We should fix it. I haven't done in-depth testing for overloaded functions yet... thanks for reminding me. :-)

@octogonz octogonz added help wanted If you're looking to contribute, this issue is a good place to start! effort: easy Probably a quick fix. Want to contribute? :-) labels Jan 27, 2019
@octogonz octogonz changed the title [API-Documenter (v7 beta)] Overloaded namespace-level functions named with '_' [api-documenter] Overloaded namespace-level functions named with '_' in YAML Jan 27, 2019
@octogonz octogonz added the priority The maintainers consider it to be an important issue. We should try to fix it soon. label Mar 10, 2019
@octogonz
Copy link
Collaborator

octogonz commented Apr 7, 2019

@AlexJerabek I finally got around to looking at this. The _1 suffix is being added intentionally to provide different UID's to distinguish the overloaded items:

YamlDocumenter.ts

  /**
   * Calculate the DocFX "uid" for the ApiItem
   * Example:  node-core-library.JsonFile.load
   */
  private _getUid(apiItem: ApiItem): string {
    let result: string = '';
    for (const hierarchyItem of apiItem.getHierarchy()) {

      // For overloaded methods, add a suffix such as "MyClass.myMethod_2".
      let qualifiedName: string = hierarchyItem.displayName;
      if (ApiParameterListMixin.isBaseClassOf(hierarchyItem)) {
        if (hierarchyItem.overloadIndex > 0) {
          qualifiedName += `_${hierarchyItem.overloadIndex}`;
        }
      }

I think the reason you're seeing it on the web page is because the same value gets added as the fullName field:

  - uid: onenote.OneNote.run_1
    summary: >-
      Executes a batch script that performs actions on the OneNote object model, using the request context of a
      previously-created API object.
    name: OneNote.run_1
    fullName: OneNote.run_1

Originally we were specifying yamlItem.fullName to be the same as yamlItem.name, but in commit 279c1ed it was changed to yamlItem.uid. The comment says "Improved rendering of class constructors" but I seem to remember this solved some other issue as well.

Maybe you could chat with @dend and determine exactly what should go in the name and fullName field for overloaded methods? (And also confirm that our uid field makes sense -- I recall that issue #881 was talking about this as well.) Or you could manually fiddle with the .yml files until you find something that works as expected.

Basically we just need to decide what the YAML file is supposed to look like, and then the fix should be quick and easy.

@natalieethell FYI

@octogonz octogonz added needs design The next step is for someone to propose the details of an approach for solving the problem and removed priority The maintainers consider it to be an important issue. We should try to fix it soon. help wanted If you're looking to contribute, this issue is a good place to start! labels Apr 7, 2019
@octogonz octogonz added the ae-ambiguous-ids This issue belongs to a set of bugs tracked by Pete label Jun 2, 2019
@octogonz
Copy link
Collaborator

octogonz commented Jun 2, 2019

This issue belongs to a family of closely related issues. I've created metaissue #1308 to come up with a unified fix that tackles them all together.

@octogonz
Copy link
Collaborator

octogonz commented Aug 8, 2019

PR #1450 fixes this (hopefully).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ae-ambiguous-ids This issue belongs to a set of bugs tracked by Pete bug Something isn't working as intended effort: easy Probably a quick fix. Want to contribute? :-) needs design The next step is for someone to propose the details of an approach for solving the problem
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants