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

Refactor `crystal tool format` command completely #7257

Merged
merged 6 commits into from Jan 10, 2019

Conversation

Projects
None yet
7 participants
@MakeNowJust
Copy link
Contributor

MakeNowJust commented Jan 3, 2019

Close #7241

Sorry, this contains some fixes and improvements:

  • Refactor by adding a new class Crystal::Command::FormatCommand.
  • Add --show-backtrace option.
  • Fix the error message on bug found.
  • Fix status-code. It returns 1 on occured some errors, otherwise it returns 0.
  • Remove --format option because it does not work entirely.
    (I believe someone will implement it.)
  • Add specs powered by FormatCommand class.

In fact this reduces src/compiler/crystal/command/format.cr file-size by 5%.

MakeNowJust added some commits Jan 3, 2019

Refactor `crystal tool format` command completely
Close #7241

Sorry, this contains some fixes and improvements:

  - Refactor by adding a new class `Crystal::Command::FormatCommand`.
  - Add `--show-backtrace` option.
  - Fix the error message on bug found.
  - Fix status-code. It returns `1` on occured some errors,
    otherwise it returns `0`.
  - Remove `--format` option because it does not work entirely.
    (I believe someone will implement it.)
  - Add specs powered by `FormatCommand` class.

In fact this reduces `src/compiler/crystal/command/format.cr` file-size by 10%.
@straight-shoota
Copy link
Member

straight-shoota left a comment

I really love this refactor and the specs ❤️

Show resolved Hide resolved spec/compiler/crystal/tools/format_spec.cr Outdated
Show resolved Hide resolved spec/compiler/crystal/tools/format_spec.cr
Show resolved Hide resolved spec/compiler/crystal/tools/format_spec.cr
Show resolved Hide resolved src/compiler/crystal/command/format.cr Outdated
Show resolved Hide resolved src/compiler/crystal/command/format.cr Outdated
@RX14

This comment has been minimized.

Copy link
Member

RX14 commented Jan 4, 2019

Looks good, overall.

MakeNowJust added some commits Jan 5, 2019

@RX14

This comment has been minimized.

Copy link
Member

RX14 commented Jan 5, 2019

It'd be nice to have different, documented status codes for each of the different types of error. 1 for formatting produced changes, 2 for bug, 3 for invalid bye sequence, etc.

Since this PR changes status codes already, I think it'd be nice to do that here.

@MakeNowJust

This comment has been minimized.

Copy link
Contributor Author

MakeNowJust commented Jan 5, 2019

@RX14 Each error can be combined (e.g. one file has syntax error, and another file invokes a bug.) So, using bit flag as status code is good. And, I think syntax error and invalid byte sequence error can be regarded same kind error on status code.

@RX14

This comment has been minimized.

Copy link
Member

RX14 commented Jan 5, 2019

@MakeNowJust status codes are typically checked from bash scripts. Decoding bitflags is difficult there. So I'd suggest not doing that and having precedence rules instead: any bug is status code 3, any invalid byte sequence or syntax error is 2. Invalid command-line arguments should probably be exit 1. exit 1 is used in a lot of places so assigning a meaning to it is probably going to be quite error-prone.

@straight-shoota

This comment has been minimized.

Copy link
Member

straight-shoota commented Jan 5, 2019

@RX14 When running with --check, should there also be a dedicated status code for "produced changes"?

Theses extended status codes don't need to go into this PR. It's an additional feature.

This doesn't really break any behaviour regarding status codes, does it? @MakeNowJust Even if it did, that doesn't mean it can't be changed again in a subsequent PR.

@MakeNowJust

This comment has been minimized.

Copy link
Contributor Author

MakeNowJust commented Jan 5, 2019

This doesn't really break any behaviour regarding status codes, does it?

It doesn't touch any behavior about status-code with --check. And, it changes status-code without --check.

I agree with @straight-shoota. This PR is ready and we can discuss about status-code in other place.

@MakeNowJust

This comment has been minimized.

Copy link
Contributor Author

MakeNowJust commented Jan 9, 2019

ping 🏓

@RX14

RX14 approved these changes Jan 9, 2019

@sdogruyol
Copy link
Member

sdogruyol left a comment

Thank you @MakeNowJust 👍

@sdogruyol sdogruyol merged commit 9913f8a into crystal-lang:master Jan 10, 2019

5 checks passed

ci/circleci: check_format Your tests passed on CircleCI!
Details
ci/circleci: test_darwin Your tests passed on CircleCI!
Details
ci/circleci: test_linux Your tests passed on CircleCI!
Details
ci/circleci: test_linux32 Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@sdogruyol sdogruyol added this to the 0.27.1 milestone Jan 10, 2019

@MakeNowJust MakeNowJust deleted the MakeNowJust:fix/refactor-format-command branch Jan 10, 2019

urde-graven pushed a commit to urde-graven/crystal that referenced this pull request Jan 12, 2019

@faustinoaq

This comment has been minimized.

Copy link
Contributor

faustinoaq commented Feb 7, 2019

  • Remove --format option because it does not work entirely.
    (I believe someone will implement it.)

Oh, this just broke vscode-crystal extension 😅 crystal-lang-tools/vscode-crystal-lang#80

I'm already working on a quick fix 💪 👍

@asterite

This comment has been minimized.

Copy link
Contributor

asterite commented Feb 7, 2019

Yeah, I don't know what happened but sublime text stooped formatting my Crystal files :-(

@asterite

This comment has been minimized.

Copy link
Contributor

asterite commented Feb 7, 2019

And sublime text wasn't using --format at all... I'll try to see today what broke.

@asterite

This comment has been minimized.

Copy link
Contributor

asterite commented Feb 7, 2019

Actually, Sublime does use --format... why was this removed without checking first if this would be a breaking change? :-(

@asterite

This comment has been minimized.

Copy link
Contributor

asterite commented Feb 7, 2019

@MakeNowJust Could you explain what you mean with this?

Remove --format option because it does not work entirely

It was used by Sublime Text (and apparently VSCode) and it was working fine. And right now we have no option to replace this (SublimeText used the line/column info from the JSON to show a dot where a line had an error, now this is impossible because we only have a plain text error).

I think it's better to have a half-baked solution that was kind of working than completely removing it with no replacement.

@asterite

This comment has been minimized.

Copy link
Contributor

asterite commented Feb 7, 2019

@faustinoaq In case you are working on a fix and wonder how to do it, I ended up doing this in Sublime Text: crystal-lang-tools/sublime-crystal#65

@MakeNowJust Don't worry too much about this, now there are fixes coming for this. We can put back the JSON output next without rush.

@faustinoaq

This comment has been minimized.

Copy link
Contributor

faustinoaq commented Feb 7, 2019

Hi @asterite VSCode supports output parsing on tasks and extensions to show errors, see: https://code.visualstudio.com/docs/editor/tasks#_defining-a-problem-matcher

However, I disabled OnFly error check (kinda workaround) to support formatting without -f json flag 😅

https://github.com/crystal-lang-tools/vscode-crystal-lang/blob/5feb5f9db9efc2ef1caaefa78d2a48eb2346946f/src/crystalFormatting.ts#L45

Thank you for your PR on sublime-crystal 👍

why was this removed without checking first if this would be a breaking change?

@MakeNowJust Yeah, this is kinda breaking change 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment