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

refactor: improve custom filters readability with optional chaining #1275

Merged

Conversation

ManikantaMandala
Copy link
Contributor

@ManikantaMandala ManikantaMandala commented Oct 3, 2024

Description
refactor customFilter.js file mentioned here: these_issues

also mentioned in this issue #1272

Copy link

changeset-bot bot commented Oct 3, 2024

⚠️ No Changeset found

Latest commit: 95dda3c

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.

@ManikantaMandala ManikantaMandala changed the title refactor: refactored the code as per refactor: refactored the code Oct 3, 2024
@ManikantaMandala ManikantaMandala marked this pull request as draft October 3, 2024 12:46
@ManikantaMandala ManikantaMandala marked this pull request as ready for review October 3, 2024 12:47
@derberg derberg changed the title refactor: refactored the code refactor: improve custom filters readability with optional chaining Oct 7, 2024
Copy link
Member

@derberg derberg left a comment

Choose a reason for hiding this comment

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

@ManikantaMandala thanks, just left one comment and also update PR title to make it more informative as later it lands as a commit message in master

@derberg
Copy link
Member

derberg commented Oct 7, 2024

@ManikantaMandala please followup with failing tests. Next time, you better run them locally to make sure they pass, otherwise you will have to do this test ping pong with maintainers and wait for GitHub Actions to run tests for you

change the file to the suggestions

Co-authored-by: Lukasz Gornicki <lpgornicki@gmail.com>
@ManikantaMandala
Copy link
Contributor Author

ManikantaMandala commented Oct 7, 2024

@ManikantaMandala please followup with failing tests. Next time, you better run them locally to make sure they pass, otherwise you will have to do this test ping pong with maintainers and wait for GitHub Actions to run tests for you

Thank you for the feedback @derberg
As this is my first contribution I had made some mistakes
I will do the necessary things for the next pr

and also can you suggest where to refer to test it locally if possible
I have done local testing https://github.com/asyncapi/generator/blob/master/Development.md#local-testing
and also done docker isolated testing.
all passed

@derberg
Copy link
Member

derberg commented Oct 8, 2024

@ManikantaMandala errors happen even if you make PR number 100, no worries

test run is visible in https://github.com/asyncapi/generator/actions/runs/11219255311/job/31244271269?pr=1275

and yes, run locally as described in development guide - if they pass locally, please share the log in this PR for overview

@derberg
Copy link
Member

derberg commented Oct 8, 2024

@ManikantaMandala because of a merge of one PR, you have conflicts here now, so please solve them and push updates to the PR

@ManikantaMandala ManikantaMandala marked this pull request as draft October 9, 2024 07:52
@ManikantaMandala ManikantaMandala marked this pull request as ready for review October 9, 2024 08:04
Copy link
Member

@derberg derberg left a comment

Choose a reason for hiding this comment

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

please remove changes from eslintrc

solved merge conflits for merging master branch
@ManikantaMandala ManikantaMandala force-pushed the refactor_customFilter.js branch from 96eac67 to e9ed098 Compare October 9, 2024 17:26
Copy link

sonarqubecloud bot commented Oct 9, 2024

@derberg
Copy link
Member

derberg commented Oct 9, 2024

/rtm

@asyncapi-bot asyncapi-bot merged commit 074e520 into asyncapi:master Oct 9, 2024
12 checks passed
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.

3 participants