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: add missing params validation in testFile_md component #1309

Closed
wants to merge 3 commits into from

Conversation

toffee-k21
Copy link

@toffee-k21 toffee-k21 commented Oct 22, 2024

Description

  • add props validation mentioned here : these_issue
  • also mentioned in this issue : #1272

Copy link

changeset-bot bot commented Oct 22, 2024

⚠️ No Changeset found

Latest commit: 1909a77

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.

@ItshMoh
Copy link

ItshMoh commented Oct 22, 2024

Hey @toffee-k21 you have changed in wrong file. You have to do in 'apps/generator/test/test-templates/react-template/template/test-file.md.js' this file but you have done in autogenerated file 'apps/generator/test/test-templates/react-template/__transpiled/test-file.md.js' In the __transpiled folder there are autogenerated files.

@toffee-k21
Copy link
Author

Hey @ItshMoh, I've moved the file implementation to the correct folder. Let me know if this looks good or if any adjustments are needed.

Comment on lines 3 to 12
const TestFile = ({ asyncapi, params }) => {
return (
<File name="test-file.md">
<Text>This is a markdown file for my application.</Text>
<Text>App name is: **{ asyncapi.info().title() }**</Text>
<Text>Version {params.version} running on {params.mode} mode </Text>
</File>
);
} No newline at end of file
}
Copy link
Contributor

Choose a reason for hiding this comment

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

How's about using function syntax as it was used before in place of arrow functions since this will maintain the code consistency of the codebase.

Copy link
Author

Choose a reason for hiding this comment

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

Implemented the feature using the 'function' keyword

Copy link

@derberg
Copy link
Member

derberg commented Oct 28, 2024

sorry but it doesn't make sense to make this change to a template that is just there for development

looks like we forgot to update sonar when we created monorepo

I will close this PR and if you really wanna help, please open a new PR and update https://github.com/asyncapi/generator/blob/master/.sonarcloud.properties with proper link to test dir in apps/generator, you can add test dir for nunjucks templates as well

@derberg derberg closed this Oct 28, 2024
@toffee-k21
Copy link
Author

sorry but it doesn't make sense to make this change to a template that is just there for development

looks like we forgot to update sonar when we created monorepo

I will close this PR and if you really wanna help, please open a new PR and update https://github.com/asyncapi/generator/blob/master/.sonarcloud.properties with proper link to test dir in apps/generator, you can add test dir for nunjucks templates as well

Hey @derberg, I've updated the paths to the test directories as requested in PR #1311. Is this the update you were looking for?

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

Successfully merging this pull request may close these issues.

4 participants