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

Implement the beautifier #1

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Implement the beautifier #1

wants to merge 11 commits into from

Conversation

lassik
Copy link

@lassik lassik commented Oct 2, 2018

  • This PR contains one test that passes.
  • elm-format doesn't have any options.
  • The current version of elm-format adds ANSI color codes to the error messages that it outputs to stderr, even when stderr is not a terminal. This is discussed in elm-format issue #372.

@Glavin001 Glavin001 self-requested a review October 2, 2018 23:53
@Glavin001
Copy link
Member

The current version of elm-format adds ANSI color codes to the error messages that it outputs to stderr, even when stderr is not a terminal. This is discussed in elm-format issue #372.

Consider using something like: https://www.npmjs.com/package/strip-ansi

Copy link
Member

@Glavin001 Glavin001 left a comment

Choose a reason for hiding this comment

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

Tests currently failing.

@Glavin001 Glavin001 added help wanted Extra attention is needed good first issue Good for newcomers labels Oct 3, 2018
@stevenzeck
Copy link

@Glavin001 please give me more access to this so I can add to CodeClimate.

@lassik
Copy link
Author

lassik commented Oct 3, 2018

It would be really nice to make do without us caring about ANSI colors. Since this is still work in progress, I'll try to persuade upstream to take care of it so we get plain text output without ANSI.

@stevenzeck
Copy link

@lassik I'll add this to the beautifier-template as well, but could you please add a .vscode/launch.json file? You can probably use https://raw.githubusercontent.com/Unibeautify/beautifier-eslint/master/.vscode/launch.json

cache:
directories:
- node_modules
before_script:

Choose a reason for hiding this comment

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

You need to install elm as well

Copy link
Author

Choose a reason for hiding this comment

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

According to the Travis build log, at least elm-format --help is running fine when we have only elm-format installed without the rest of the Elm toolchain. Does it show an error somewhere?

Choose a reason for hiding this comment

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

Yeah, the error is being written to stdout for some reason.

"I couldn't figure out what version of Elm you are using!

You should either run elm-format from the directory containing
your elm.json (for Elm 0.19) or elm-package.json (for Elm 0.18),
or tell me which version of Elm with --elm-version=0.19 or --elm-version=0.18

"

Copy link
Author

Choose a reason for hiding this comment

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

Awesome that you noticed that, I didn't notice at all and still can't find it :-D Tried grepping the raw logs of the latest build. Anyway, I think it's saying we need an Elm project file or a command line option to let it know the Elm version. Probably we don't actually need to have that version of Elm installed. I'll try adding --elm-version= to the unit test.

Copy link
Author

Choose a reason for hiding this comment

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

If it's reporting an error to stdout when you try to format stdin->stdout, we need to file a bug report about that..

Choose a reason for hiding this comment

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

I'll try adding --elm-version= to the unit test.

I would send the projectPath into the beautify method, then include cwd: projectPath in the options

Copy link
Author

Choose a reason for hiding this comment

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

That might work. The latest version of elm-format supports both Elm 0.18 and Elm 0.19. It determines the version based on the presence of elm-package.json (0.18) or elm.json (0.19) in the current directory. That means strictly the current directory only - it doesn't look in parent directories. There's also a command line flag to control the Elm version but that seems like a more difficult solution for end users.

Copy link

Choose a reason for hiding this comment

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

I've been debating whether elm-format should check the elm version for each file starting at the file and traversing up directories until it finds elm.json or elm-package.json, instead of looking in the current directory. It seems like that would probably be more correct for more cases. What do you think? (In any case, that would be in elm-format 0.8.2 at the earliest)

Copy link
Author

Choose a reason for hiding this comment

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

Great to see you here @avh4. You really care about your project! You already seem to have an issue about this at avh4/elm-format#561 so let's continue discussion there to make sure everyone can find it :) For this plugin, the most useful missing elm-format feature would be using isatty() (in Haskell, queryTerminal) to disable ANSI output when stderr is not going a terminal.

Copy link
Author

Choose a reason for hiding this comment

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

Discussion about Elm version number is ongoing in #2

@Glavin001
Copy link
Member

@stevenzeck Try now, you should be an Admin 👍 .

.travis.yml Outdated
@@ -0,0 +1,35 @@
env:
global:
- CC_TEST_REPORTER_ID=

Choose a reason for hiding this comment

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

The ID is: 0288dbd5e8a04d83f7e4e86710d77c540968fc4e791afe95e5d3aa79e20b90f9

Copy link
Author

Choose a reason for hiding this comment

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

Thanks! Committed

.travis.yml Outdated Show resolved Hide resolved
.travis.yml Outdated Show resolved Hide resolved
LICENSE Outdated Show resolved Hide resolved
Copy link
Member

@Glavin001 Glavin001 left a comment

Choose a reason for hiding this comment

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

Tests running within Travis CI are currently failing.

@lassik
Copy link
Author

lassik commented Oct 11, 2018

To make the test pass, we need to decide how to pass the Elm version number.

@stevenzeck
Copy link

Until we do a language version option in Unibeautify core, I think you'll need to have those config json files.

@lassik
Copy link
Author

lassik commented Oct 11, 2018

Yeah, I guess we could add an empty elm.json file into the test fixtures to solve the immediate issue.

@lassik
Copy link
Author

lassik commented Oct 11, 2018

Then again, maybe the failing test is a good reminder that we have to solve the problem properly at some point.

@Glavin001
Copy link
Member

Yeah, I guess we could add an empty elm.json file into the test fixtures to solve the immediate issue.

It may take a little while before I have time to implement Unibeautify/unibeautify#196

I think adding a note in this README that elm.json is required should be sufficient for now, and we can improve this experience in an upcoming release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants