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

DocComments should support "@prop <propName>" inside comments on model elements to make documenting a model a single block comment #2211

Closed
garethj-msft opened this issue Jul 26, 2023 · 8 comments · Fixed by #3527
Assignees
Labels
design:accepted Proposal for design has been discussed and accepted. triaged:core
Milestone

Comments

@garethj-msft
Copy link
Member

garethj-msft commented Jul 26, 2023

Graph API reviewers prefer the following syntax for authoring and reviewing to provide greater visual scanning on read (at the cost of editing in two places for updates).

/**
 * This object represents what actions are allowed on a task, such as move or delete.
 * It derives from `plannerPropertyRule`.
 * All values that are null use the value of the parent entity's `defaultRule` field.
 * @prop move Where can the task be moved valid values are `allow`, `moveBetweenBuckets`, `moveBetweenPlans`, and `block`.
 * @prop title Valid values are `block` or `allow`. `block` indicates edits are blocked `allow` indicates there are no restrictions.   
 * @prop notes Valid values are `block` or `allow`. `block` indicates edits are blocked `allow` indicates there are no restrictions.
 * @prop references Valid values are `add`, `delete`, `update`, `block`, and `allow`.   
 */
@complex model plannerTaskPropertyRule {
  @workloadName("Move") move: string[];
  @workloadName("Title") title: string[];
  @workloadName("Notes") notes: string[];
  @workloadName("References") references: plannerFieldRules;
}

This would be semantically equivalent to:

/**
 * This object represents what actions are allowed on a task, such as move or delete.
 * It derives from `plannerPropertyRule`.
 * All values that are null use the value of the parent entity's `defaultRule` field.
 */
@complex model plannerTaskPropertyRule {
  /** Where can the task be moved valid values are `allow`, `moveBetweenBuckets`, `moveBetweenPlans`, and `block`. */
  @workloadName("Move") move: string[];

  /** Valid values are `block` or `allow`. `block` indicates edits are blocked `allow` indicates there are no restrictions. */
  @workloadName("Title") title: string[];

  /** Valid values are `block` or `allow`. `block` indicates edits are blocked `allow` indicates there are no restrictions. */
  @workloadName("Notes") notes: string[];

  /** Valid values are `add`, `delete`, `update`, `block`, and `allow`. */  
  @workloadName("References") references: plannerFieldRules;
}

Similarly for enums, @member <membername> embeddings should be supported for consistency of commenting style across structured elements.

@timotheeguerin
Copy link
Member

timotheeguerin commented Jul 27, 2023

All those questions are probably going to get resolved tomorrow during the design meeting for @param #1946 and we most likely have the same logic here too

But need to make sure we have a consistent logic for the following:

model A {
  /** Doc comment */
  a: string
}

/** 
 * @prop a override a doc for b
*/
model B {
  ...A
}

I would expect B.a to have override a doc for b as doc

model A {
  /** Doc comment */
  a: string
}

/** 
 * @prop a override a doc for b
*/
model B {
  ...A
}

@@doc(B.a, "Override again")

I would expect B.a to have Override again as doc

@markcowl markcowl added this to the Backlog milestone Jul 31, 2023
@markcowl markcowl added design:needed A design request has been raised that needs a proposal Graph Impact and removed needs-area labels Jul 31, 2023
@markcowl markcowl modified the milestones: Backlog, [2024] April Mar 11, 2024
@markcowl
Copy link
Contributor

Related to #2233

@markcowl markcowl added design:accepted Proposal for design has been discussed and accepted. and removed design:needed A design request has been raised that needs a proposal labels Mar 12, 2024
@markcowl markcowl modified the milestones: [2024] April, Backlog Mar 12, 2024
@garethj-msft
Copy link
Member Author

Yay - great to see this moving - we've really sold it to teams for Graph.

@garethj-msft
Copy link
Member Author

@mario-guerra any chance we can get this prioritized? We're trying hard to sell TypeSpec as master to our docs team and everyone really wants this.

@mario-guerra
Copy link
Member

Pinging @allenjzhang for comment on prioritization.

@timotheeguerin
Copy link
Member

Pr fixing the issue with @param is here #3517 this means the same logic should be able to be reused for @prop

@timotheeguerin
Copy link
Member

timotheeguerin commented Jun 4, 2024

model Base {
  /** Doc comment on prop */
  prop: string;
}
// getDoc(Base.prop) === "Doc comment on prop"
model Base {
  /** Doc comment on prop */
  @doc("Explicit")
  prop: string;
}
// getDoc(Base.prop) === "Explicit"
/**
 * @prop prop Doc comment from model
 */
model Base {
  prop: string;
}
// getDoc(Base.prop) === "Doc comment from model"
/**
 * @prop prop Doc comment from model
 */
model Base {
  /** Doc comment on prop */
  prop: string;
}
// getDoc(Base.prop) === "Doc comment on prop"
model Base {
  /** Doc comment on prop */
  prop: string;
}

/**
 * @prop prop Doc comment from model override
 */
model Child {
  ...Base
}
// getDoc(Child.prop) === "Doc comment from model override"
model Base {
  /** Doc comment on prop */
  prop: string;
}

/**
 * @prop prop Doc comment from model override
 */
model Child {
  ...Base
}
@@doc(Child.prop, "override via augment")
// getDoc(Child.prop) === "override via augment"

@timotheeguerin
Copy link
Member

PR #3527

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design:accepted Proposal for design has been discussed and accepted. triaged:core
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants