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

style(core): reformat all of packages/core with new clang format version #36549

Closed
wants to merge 1 commit into from

Conversation

kara
Copy link
Contributor

@kara kara commented Apr 9, 2020

This commit updates the formatting of all packages/core/*
files at once to comply with the new version of clang.
Doing it all at once this way will cause some short-term
rebasing of existing core PRs, but will prevent head-
aches when reviewing PRs down the line (because we
will not have to filter out the noise of unrelated
formatting changes as these files are touched in the
future).

@kara kara added the area: core Issues related to the framework runtime label Apr 9, 2020
@ngbot ngbot bot added this to the needsTriage milestone Apr 9, 2020
@kara kara added the target: patch This PR is targeted for the next patch release label Apr 9, 2020
@kara kara force-pushed the reformat-core branch 5 times, most recently from 671e322 to 1e4c605 Compare April 9, 2020 22:32
This commit updates the formatting of all packages/core/*
files at once to comply with the new version of clang.
Doing it all at once this way will cause some short-term
rebasing of existing core PRs, but will prevent head-
aches when reviewing PRs down the line (because we
will not have to filter out the noise of unrelated
formatting changes as these files are touched in the
future).
@kara kara marked this pull request as ready for review April 9, 2020 23:32
@pullapprove pullapprove bot requested review from IgorMinar and mhevery April 9, 2020 23:32
@kara kara added the action: review The PR is still awaiting reviews from at least one requested reviewer label Apr 9, 2020
Copy link
Contributor

@IgorMinar IgorMinar left a comment

Choose a reason for hiding this comment

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

Can we please do this ones across the whole repo and not just within packages/core as the entire repo is affected in the same way?

Additionally we should consider writing a script that would go over all the existing PRs that currently don't have a merge conflict and:

  • check them out
  • rebase them on the latest master
  • reformat them
  • add a "style:" commit with the reformatting diff
    -and force push to the PR branch

That way we know that all the existing PRs that are currently in a good enough shape, will remain in a good enough shape.

This will still leave us with a lot of PRs that currently have a merge conflict and can't be cleanly rebased, in which case we need to come up with a different reformatting strategy for these....

@kara
Copy link
Contributor Author

kara commented Apr 9, 2020

@IgorMinar In the Monday sync, we agreed to do the repo in smaller pieces (see #36520 for another example). Let's chat offline.

@IgorMinar IgorMinar removed state: blocked action: review The PR is still awaiting reviews from at least one requested reviewer labels Apr 10, 2020
Copy link
Contributor

@IgorMinar IgorMinar left a comment

Choose a reason for hiding this comment

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

We discussed this with @kara and @josephperrott in person and agreed that the best course of action is to merge this PR and schedule follow up work to reformat the rest of the monorepo.

@josephperrott is looking into possibility to update the PRs automatically in the future, but we will not block this PR on that.

@josephperrott is also going to look into using prettier instead.

@IgorMinar IgorMinar removed the request for review from mhevery April 10, 2020 00:35
@pullapprove pullapprove bot requested a review from mhevery April 10, 2020 00:36
Copy link
Contributor

@IgorMinar IgorMinar left a comment

Choose a reason for hiding this comment

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

Reviewed-for: global-approvers

@IgorMinar IgorMinar removed the request for review from mhevery April 10, 2020 00:41
@IgorMinar
Copy link
Contributor

one more update: we decided to hold off on this PR until tomorrow, while @josephperrott and I are investigating feasibility of switching over to prettier and reformatting the whole repo with it.

@kara kara closed this Apr 10, 2020
@kara
Copy link
Contributor Author

kara commented Apr 10, 2020

Update: we're going to switch to Prettier, so this PR is out of date

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators May 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: core Issues related to the framework runtime cla: yes state: blocked target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants