Skip to content

[core] Better Windows support for the API generation#12584

Merged
oliviertassinari merged 3 commits into
mui:masterfrom
adeelibr:feature-build-conversion
Aug 20, 2018
Merged

[core] Better Windows support for the API generation#12584
oliviertassinari merged 3 commits into
mui:masterfrom
adeelibr:feature-build-conversion

Conversation

@adeelibr

Copy link
Copy Markdown
Contributor

This is to convert CRLF to LF format when you run yarn docs:api so that the CircleCI pipeline can work smoothly.

@adeelibr

Copy link
Copy Markdown
Contributor Author

Running a test, once this passes. I'll run docs:api command to generate new docs. And push it to this branch. If that passes. Then this PR is ready.

@adeelibr

Copy link
Copy Markdown
Contributor Author

This is for a personal note: The first build passed, after I updated the regex expression. Now will make a second commit to this branch with the updated docs in LF format. By running yarn docs:api

@adeelibr

Copy link
Copy Markdown
Contributor Author

I have checked, all the files that were committed in the docs api folder are of LF type now.

@eps1lon

eps1lon commented Aug 19, 2018

Copy link
Copy Markdown
Member

I think we should enforce LF as line-endings for local development. This change can (and did in this PR) introduce CRLF into the generated markdown which will cause CI failures and noisy diffs.

@oliviertassinari

oliviertassinari commented Aug 19, 2018

Copy link
Copy Markdown
Member

I think we should enforce LF as line-endings for local development.

@eps1lon As far as I remember, we have already tried that. It's a dead end.
https://github.com/mui-org/material-ui/blob/master/.gitattributes

@eps1lon

eps1lon commented Aug 20, 2018

Copy link
Copy Markdown
Member

I think some parser is actually introducing CLRF as line-endings because it runs on windows. It's not actually detecting what line-ending is used. For example https://github.com/mui-org/material-ui/blob/905a96119e474731b8e7add6320d3484377bf242/docs/src/modules/utils/generateMarkdown.js#L76 tag.description will contain CLRF even if the actual source has only LF.

We need to investigate how we can normalize the markdown generation across platforms. Otherwise this will always cause noisy diffs and CI fails.

@adeelibr

Copy link
Copy Markdown
Contributor Author

I tried searching it else-where as well, where ever \n this regex expression was being used while markdown generation and the only file where this is being used is docs/src/modules/utils/generateMarkdown.js on line 76 as @eps1lon mentioned. Should I update this regex pattern as well?

@eps1lon

eps1lon commented Aug 20, 2018

Copy link
Copy Markdown
Member

That won't be enough I think. We also need to be able to tell react-docgen to pass \n as a lineTerminator option to recast which is not possible at the moment.

I think the best option is to let a maintainer run yarn docs:api on PRs that are created by windows contributors.

Is adding a linux environment for development not an option for @adeelibr ?

@adeelibr

Copy link
Copy Markdown
Contributor Author

@eps1lon I think for now maybe the maintainer can run yarn docs:api for windows user, what i'll do at my end is install a portable linux version on my system. So that I can contribute more effectively. :)

Your thoughts @oliviertassinari ?

@oliviertassinari oliviertassinari self-assigned this Aug 20, 2018
@oliviertassinari oliviertassinari changed the title updated build script [core] Better windows support for the API generation Aug 20, 2018
@oliviertassinari
oliviertassinari force-pushed the feature-build-conversion branch from 8560ecb to aeabf26 Compare August 20, 2018 17:32
@oliviertassinari

oliviertassinari commented Aug 20, 2018

Copy link
Copy Markdown
Member

I have tried to quickly set up a Windows environment with no luck. I wish the Windows support story was better than that.

capture d ecran 2018-08-20 a 19 37 20
https://insights.stackoverflow.com/survey/2018/

@oliviertassinari oliviertassinari changed the title [core] Better windows support for the API generation [core] Better Windows support for the API generation Aug 20, 2018
@adeelibr

Copy link
Copy Markdown
Contributor Author

So what's the next step that I should take. @oliviertassinari

Since my other PR #12485 is now very messy with a lot of docs merges, should I delete that one & create a new one. With just the changes that are required there..

@oliviertassinari
oliviertassinari merged commit 616efbc into mui:master Aug 20, 2018
@oliviertassinari

Copy link
Copy Markdown
Member

@adeelibr I'm merging this pull request as it's a good step forward.

So what's the next step that I should take.

I would say fixing the Windows support would be awesome.

should I delete that one & create a new one.

No need, you can push force on it, erasing the history.

@adeelibr

Copy link
Copy Markdown
Contributor Author

@oliviertassinari if it's okay, can i make an extra experimental branch for this Windows support feature for docs generation to experiment, in order to check if the build pipeline works with those changes of mine or not. Would that be okay?

@adeelibr
adeelibr deleted the feature-build-conversion branch August 20, 2018 19:22
@adeelibr

Copy link
Copy Markdown
Contributor Author

Would adding another property in .gitattributes help with this issue here

* text=auto eol=lf

Reference https://stackoverflow.com/a/42136008/4921319

@oliviertassinari

oliviertassinari commented Jan 27, 2019

Copy link
Copy Markdown
Member

@adeelibr The problem has been fixed as far as I know. I'm not sure eol:lf will help as I would expect windows user's editor to still use crlf.
https://help.github.com/articles/dealing-with-line-endings/

@joshwooding

joshwooding commented Jan 27, 2019

Copy link
Copy Markdown
Collaborator

AFAIK IntelliJ copies the first line ending it sees in the file but I don’t see a need atm for setting a line ending

@zannager zannager added the internal Behind-the-scenes enhancement. Formerly called “core”. label Mar 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

internal Behind-the-scenes enhancement. Formerly called “core”.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants