Made line-ending a warning. It's still an annoyance for windows user… #839
Conversation
…, but it doesn't stop scripts running. Added .cmd extents to allow examples.js to run on windows. Made the hello world example actually use a non-standard locale so that you can see that something is happening.
|
Thank you for submitting this pull request, however I do not see a valid CLA on file for you. Before we can merge this request please visit https://yahoocla.herokuapp.com/ and agree to the terms. Thanks! 😄 |
|
If you mean the changes to examples/hello-world/src/index.js and App.js , I made those because without them it was not obvious to me that the internationalisation was working on Windows - ie. no translation was being performed. So if we modify the scope slightly to "make the examples obviously work on Windows" then I think it's in scope. I guess if you don't want to "prefer" French in any way, you could change index.js to render The examples/hello-world changes aren't necessary, but I felt they would add to the understanding of a new windows developer coming to the project. I won't be at all upset if you just take the other changes! |
Added missing semi-colon (sorry - my local build and tests didn't complain!)
There was a problem hiding this comment.
@OracPrime thanks for doing this! I'd like to keep this PR specific for making the examples work on Windows. So I'd prefer to not make changes to any of the examples in this PR.
I added a few line comments as well.
| @@ -22,7 +22,7 @@ | |||
| "eqeqeq": 2, | |||
| "eol-last": 2, | |||
| "indent": [2, 4], | |||
| "linebreak-style": [2, "unix"], | |||
| "linebreak-style": [1, "unix"], | |||
There was a problem hiding this comment.
Re: your comment in the issue about Windows and Git… can you explain a bit more of what's happening now? Is there a way to have a Windows environment that uses unix line endings?
There was a problem hiding this comment.
When git pulls from GitHub to Windows, it converts the linebreaks to Windows-style. When you push back from Windows it converts back. So (at least in theory) Windows and Linux developers each see their own line endings, but the repository only sees Linux line endings. So having Linux and the CI builds check line endings is fine, but it breaks the tests when run by a Windows developer. There are discussions at eslint about having environment-dependent rules, but they haven't come to anything yet. So if we make it a warning, the CI and Linux developers will see there is a problem (if it gets past the auto-convert from Git) in that they will get any warnings at all. Windows developers, second rate citizens that we are :), will just have to ignore the multiple-thousand warnings, but at least it won't make pre-commit tests fail.
There was a problem hiding this comment.
Okay, cool. So what you have seems like the best solution until ESLint does something about it.
examples/hello-world/src/App.js
Outdated
| import React, {Component} from 'react'; | ||
| import {FormattedMessage} from 'react-intl'; | ||
| import React, { Component } from 'react'; | ||
| import { FormattedMessage, FormattedRelative } from 'react-intl'; |
There was a problem hiding this comment.
Can you revert these formatting changes?
examples/hello-world/src/App.js
Outdated
| values={{ name: <b>{name}</b>, unreadCount }} | ||
| /> | ||
| </p> | ||
| <p> Last update <FormattedRelative value={Date.now() - 3600000} /> </p> |
There was a problem hiding this comment.
Using Date.now() inside render() is an anti-pattern that I don't what to encourage in the examples.
There was a problem hiding this comment.
Good point. I'll slap my own wrist for that one. And revert all the hello-world changes.
examples/hello-world/src/index.js
Outdated
| import App from './App'; | ||
|
|
||
| import fr from 'react-intl/locale-data/fr'; | ||
| addLocaleData(fr); |
There was a problem hiding this comment.
I want to avoid using more than one language for the hello-world example. React Intl is still useful even if you're only using English. In this example the numbers are formatted and there's plurals.
There was a problem hiding this comment.
Fair enough. Maybe when someone has time we can add a multi-language demo which demonstrates multiple languages on one page. Although the irony of wanting Hello World to be in a single language is not lost on me :)
scripts/examples.js
Outdated
| @@ -8,8 +8,8 @@ globSync('./examples/*/').forEach((exampleDir) => { | |||
| cwd: exampleDir, | |||
| stdio: 'inherit', | |||
| }; | |||
|
|
|||
| const result = spawnSync(command, args, opts); | |||
| let envCommand=command+ (/^win/.test(process.platform) ? '.cmd' : ''); | |||
There was a problem hiding this comment.
I think this formatting is a bit confusing. Can you switch it to this (spaces around = and +:
let envCommand = command + (/^win/.test(process.platform) ? '.cmd' : '');There was a problem hiding this comment.
I'm also talking to the cross-env guys about why the nested example fail on Windows. It's not clear whether cross-env or cross-spawn needs correcting, but the fault lies there, not here.
There was a problem hiding this comment.
OK, I think all those changes are in now.
Clarified formatting in examples.js
|
@OracPrime thanks! Merged! |
…s, but it doesn't stop scripts running.
Added .cmd extents to allow examples.js to run on windows.
Made the hello world example actually use a non-standard locale so that you can see that something is happening.