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

fix: generate doc for getters #1780

Merged
merged 7 commits into from
Feb 15, 2024

Conversation

akkshitgupta
Copy link
Collaborator

@akkshitgupta akkshitgupta commented Feb 4, 2024

Description

This pull request corrects the generated code docs to their expected position.

Earlier Now
Screenshot-1 Screenshot-2

Related Issue

Fixes #1548

Checklist

  • The code follows the project's coding standards and is properly linted (npm run lint).
  • Tests have been added or updated to cover the changes.
  • Documentation has been updated to reflect the changes.
  • All tests pass successfully locally.(npm run test).

Copy link

netlify bot commented Feb 4, 2024

Deploy Preview for modelina canceled.

Name Link
🔨 Latest commit e9c692b
🔍 Latest deploy log https://app.netlify.com/sites/modelina/deploys/65cdcf43b6230600082634a0

@coveralls
Copy link

coveralls commented Feb 4, 2024

Pull Request Test Coverage Report for Build 7913250574

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 92.326%

Totals Coverage Status
Change from base Build 7913190707: 0.0%
Covered Lines: 5993
Relevant Lines: 6324

💛 - Coveralls

@akkshitgupta
Copy link
Collaborator Author

Hey @Samridhi-98 can you please review this pull request. Thank You 😄

@@ -37,7 +37,7 @@ export const TS_DESCRIPTION_PRESET: TypeScriptPreset = {
self({ renderer, model, content }) {
return renderDescription({ renderer, content, item: model });
},
property({ renderer, property, content }) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to make it slightly more advanced, and use the options argument to conditionally write docs for properties or getters depending on whether the option for using getters is sat 🙂

Copy link
Collaborator Author

@akkshitgupta akkshitgupta Feb 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could not understand from the given issue description, unfortunately.

Can you confirm whether the option we are referring to is passed from the playground UI just like includeDescription or there is any other way for that.

export interface ModelinaTypeScriptOptions extends ModelinaGeneralOptions {
  tsMarshalling: boolean;
  tsModelType: 'class' | 'interface' | undefined;
  tsEnumType: 'union' | 'enum' | undefined;
  tsMapType: 'indexedObject' | 'map' | 'record' | undefined;
  tsModuleSystem: 'ESM' | 'CJS' | undefined;
  tsIncludeDescriptions: boolean;
  tsIncludeJsonBinPack: boolean;
  tsIncludeExampleFunction: boolean;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the late reply @akkshitgupta 🙇

So actually I see that it's quite hard to check whether we are using getters or properties, cause it's determined based on the presets that the user includes.

As a quick workaround, lets add the description both for getter and property (does not matter if there are duplicate information for now ✌️

Copy link
Collaborator Author

@akkshitgupta akkshitgupta Feb 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the late reply @akkshitgupta 🙇

I understand that you are occupied with your work as well. Thank you for being helpful as it encourages me to accept more challenges and continue my journey with AsyncAPI.

does not matter if there are duplicate information for now.

I would like to solve it out rather than just allowing the duplicates if it not much necessary to include description for getters and hold up the PR for now. LMK WDYT

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am getting so confused with all these PRs targeting different languages where some use getters and setters automatically and others have options to turn them on and off..

For TypeScript it's always on and can't be turned off... So your implementation is excellent 😄

Copy link
Member

@jonaslagoni jonaslagoni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for taking this long to end up with nothing but smoke 🙇

@jonaslagoni
Copy link
Member

/rtm

@jonaslagoni
Copy link
Member

jonaslagoni commented Feb 13, 2024

@Samridhi-98 do you have time to take a look ✌️

@Samridhi-98
Copy link
Collaborator

/rtm

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
7.9% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

@jonaslagoni jonaslagoni merged commit 8598e34 into asyncapi:master Feb 15, 2024
16 of 17 checks passed
@jonaslagoni
Copy link
Member

@all-contributors please add @akkshitgupta for code, bugs

Copy link
Contributor

@jonaslagoni

I've put up a pull request to add @akkshitgupta! 🎉

@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 3.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 4.0.0-next.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TypeScript include documentation for properties instead of getters
5 participants