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 bind multiple Contribution using for of instead foreach #9148

Closed
wants to merge 1 commit into from

Conversation

hogod
Copy link
Contributor

@hogod hogod commented Mar 3, 2021

Fix to use for of instead Array.foreach when bind multiple Contribution in frontend module in common

Signed-off-by: Byeongho Jung hobeoung2@gmail.com

What it does

When bind multiple contribution someone use for of (ex. monaco-frontend-module, terminal-frontend-module, etc) and others use foreach (ex. frontend-application-module, editor-frontend-module). So, I fix them to use for of in common. Cause this logic bind designated Contribution rather than processing Array element, I think it is natural to use for of.

How to test

I Just check working well and check correct parameter passed to bind() using logging.

Review checklist

Reminder for reviewers

Fix to use `for of` instead `Array.foreach` when bind multiple Contribution in frontend module in common

Signed-off-by: Byeongho Jung <hobeoung2@gmail.com>
@hogod
Copy link
Contributor Author

hogod commented Mar 3, 2021

I test on MacOS and Ubuntu 20.04 before pull request, and try to find ubuntu build problem now.

@hogod
Copy link
Contributor Author

hogod commented Mar 3, 2021

I Cannot Reproduce in my ubuntu 18.04 docker environment either.
But, I Found Same Error in #9100, #9081, Can I get the reason about this problem or Clue to Analysis this Problem?

@vince-fugnitto
Copy link
Member

I Cannot Reproduce in my ubuntu 18.04 docker environment either.
But, I Found Same Error in #9100, #9081, Can I get the reason about this problem or Clue to Analysis this Problem?

@hogod those other pull-requests likely fail since they are not based on the latest master which has a fix for the intermittent failures when testing the browser application (api-tests).

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

@hogod thank you for the contribution, was there a specific reason the changes were proposed?
I'm not sure the updates are necessary or add much value, nor is there a fixed convention (as described in the coding guidelines) for using for of instead of forEach when binding.

@hogod
Copy link
Contributor Author

hogod commented Mar 3, 2021

My purpose is to achieve same operation, same code for pretty.

Previously, two styles were mixed.

for (const identifier of [CommandContribution, KeybindingContribution, QuickOpenContribution]) { bind(identifier).toService(EditorContribution); }

[CommandContribution, KeybindingContribution, MenuContribution].forEach(serviceIdentifier => bind(serviceIdentifier).toService(QuickCommandFrontendContribution) );

And I agree with your opinion that it is not really necessary or add much value.

Thanks for the review!

@vince-fugnitto
Copy link
Member

...same code for pretty.
And I agree with your opinion that it is not really necessary or add much value.

Since we agree that the changes are not necessary, do not add value and are only for "pretty", should we close the pull-request?
There is no eslint rules, or guidelines on enforcing the convention so there is no guarantee that new contributions will not deviate.

@hogod hogod closed this Mar 3, 2021
@hogod
Copy link
Contributor Author

hogod commented Mar 3, 2021

Okay, I'll close this pull request.

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.

2 participants