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

build: workaround to run presubmit.sh on Windows #11096

Merged
merged 1 commit into from Aug 26, 2016

Conversation

marclaval
Copy link
Contributor

This PR is a workaround to be able to run ./presubmit.sh successfully on Windows.

There is a conflict on this precise line of code. On windows, clang-format wants to split it on 2 lines. On Mac it wants to keep it on one line.
Making the line one character longer brings the two to an agreement: 2 lines.

@mprobst this is still happening with version 1.0.43 of clang-format.
On top of that, this version has issues with back-tick strings. It tries to format the content and does it in different ways depending on the OS.

@marclaval marclaval added action: review action: review The PR is still awaiting reviews from at least one requested reviewer and removed action: review labels Aug 26, 2016
@@ -1220,8 +1220,9 @@ Can't have multiple template bindings on one element. Use only one attribute nam
});

it('should report when mix of template and *attrs are used on the same element', () => {
expect(() => parse('<div template="ngIf" *ngFor>', [])).toThrowError(`Template parse errors:
Copy link
Contributor

Choose a reason for hiding this comment

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

alternatively, you can also disable clang-format for a section:

// clang-format off
code goes here
// clang-format on

@mprobst
Copy link
Contributor

mprobst commented Aug 26, 2016

LGTM

@mprobst mprobst added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Aug 26, 2016
@mprobst mprobst self-assigned this Aug 26, 2016
@mprobst
Copy link
Contributor

mprobst commented Aug 26, 2016

Re formatting issues, just speculating, maybe this is caused by your git checkout using windows line endings, and clang-format miscounts \r\n as two characters, or something like that.

Can you be a bit more precise in what you mean with "On top of that, this version has issues with back-tick strings. It tries to format the content and does it in different ways depending on the OS."?

@marclaval
Copy link
Contributor Author

On a Mac, I've tried to upgrade to 1.0.43, here are the results: https://github.com/mlaval/angular/tree/clang1.0.43
It changes many back-tick strings.

If I take this branch on a Windows machine and run clang-format, it creates even more changes on these strings (can't share them right now).

I'll take another look at Windows environment next week. It could definitely be a line ending thing.
But the back-tick is another story.

@mprobst
Copy link
Contributor

mprobst commented Aug 26, 2016

I see. Those changes are WAI, clang-format now supports formatting code inside of template literals (e.g. adding whitespace around the + in ${x+1}).

Can you produce a minimal example of a source code snippet that formats differently on Windows vs Mac / Linux?

@vicb vicb merged commit 0cf5ece into angular:master Aug 26, 2016
@marclaval
Copy link
Contributor Author

@mprobst you are right, EOL was an issue. Forcing LF on Windows solves the problem that I had. This PR was in fact useless.

About clang 1.0.43, I can still see differences between Mac and Windows, even with the right EOL.
I've done the update again in a branch: https://github.com/mlaval/angular/commits/clang1.0.43
There are 4 commits:

  • the 'WIP' commit is a first formatting done on a Mac
  • the result was moved to a Windows machine where more formatting was needed, ('Windows diff' commit)
  • then back on a Mac, even more formatting needed ('Mac diff' commit)
  • finally, back on Windows again, one more round of formatting needed, exactly reverting what was done on the Mac ('Windows diff Integrate dartanalyzer into build #2' commit)

@mprobst
Copy link
Contributor

mprobst commented Aug 29, 2016 via email

@marclaval
Copy link
Contributor Author

marclaval commented Aug 29, 2016

I added one commit with 2 repro cases for 1.0.43: marclaval@9f705e1

Both were formatted on Windows, and both are not considered correctly formatted on Mac.
The reverse is also true.

@mprobst
Copy link
Contributor

mprobst commented Aug 29, 2016

Thanks Marc, I'll TAL!

Marc Laval notifications@github.com schrieb am Mo., 29. Aug. 2016 um
09:15 Uhr:

I added one commit with 2 repro cases: mlaval@9f705e1
marclaval@9f705e1

Both were formatted on Windows, and both are not considered correctly
formatted on Mac.
The reverse is also true.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#11096 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAAcKBGqDxHZAlYgRmZfAonKKe8AuHI4ks5qkwWLgaJpZM4Jt6Jo
.

@marclaval marclaval deleted the workaroundClangWindows branch December 11, 2017 09:42
@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 Sep 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants