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

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

Merged
merged 3 commits into from
Aug 20, 2018

Conversation

adeelibr
Copy link
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
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
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
Contributor Author

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

@eps1lon
Copy link
Member

eps1lon commented Aug 19, 2018

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
Copy link
Member

oliviertassinari commented Aug 19, 2018

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
Copy link
Member

eps1lon commented Aug 20, 2018

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
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
Copy link
Member

eps1lon commented Aug 20, 2018

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
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
Copy link
Member

oliviertassinari commented Aug 20, 2018

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
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
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
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
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
Copy link
Member

oliviertassinari commented Jan 27, 2019

@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
Copy link
Member

joshwooding commented Jan 27, 2019

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 core Infrastructure work going on behind the scenes label Mar 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants