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

Fix non-coloured tuistenv output #493

Merged
merged 6 commits into from Sep 18, 2019

Conversation

@pepibumur
Copy link
Contributor

pepibumur commented Sep 8, 2019

Resolves #475

Short description 📝

As @kwridan pointed out here, the format of Tuist is lost when we run it through tuistenv. After some investigation and the help from some folks in the community, it turns out the Process that we run in tuistenv is not recognized as a tty and therefore the formatting is trimmed.

Solution 📦

Following a suggestion, this PR changes the implementation of the printer to apply colours if:

  • The TUIST_COLOURED_OUTPUT env variable is set to true OR
  • The standard output & error are a tty.

When tuistenv runs Tuist, we'll set those the TUIST_COLOURED_OUTPUT variable accordingly.

Implementation 👩‍💻👨‍💻

  • Rename EnvironmentController to Environment.
  • Add Environment.shouldOutputBeColoured variable.
  • Update the printer to use the variable.
  • Update the CommandRunner to pass the TUIST_COLOURED_OUTPUT environment variable when triggering the process.
@pepibumur pepibumur self-assigned this Sep 8, 2019
@pepibumur pepibumur changed the title [WIP] Fix non-coloured tuistenv output Fix non-coloured tuistenv output Sep 13, 2019
@pepibumur pepibumur requested review from kwridan and tuist/core Sep 13, 2019
@pepibumur pepibumur marked this pull request as ready for review Sep 13, 2019
@pepibumur

This comment has been minimized.

Copy link
Contributor Author

pepibumur commented Sep 13, 2019

@tuist/core this PR is ready for review. tuist executed through tuistenv should format the output accordingly.

@xcoderobot

This comment has been minimized.

Copy link

xcoderobot commented Sep 13, 2019

1 Warning
⚠️ Have you introduced any user-facing changes? If so, please take some time to update the documentation. Keeping the documentation up to date makes it easier for users to learn how to use Tuist.

Generated by 🚫 Danger

@codecov

This comment has been minimized.

Copy link

codecov bot commented Sep 13, 2019

Codecov Report

Merging #493 into master will decrease coverage by 0.4%.
The diff coverage is 37.65%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #493      +/-   ##
==========================================
- Coverage   92.59%   92.19%   -0.41%     
==========================================
  Files         351      352       +1     
  Lines       18344    18439      +95     
==========================================
+ Hits        16986    16999      +13     
- Misses       1358     1440      +82
Impacted Files Coverage Δ
...IntegrationTests/Generator/Mocks/MockPrinter.swift 0% <ø> (ø) ⬆️
Sources/TuistCore/Utils/ColorizeSwift.swift 0% <0%> (ø)
...urces/TuistCoreTesting/Utils/MockFileHandler.swift 90.9% <0%> (ø) ⬆️
Sources/TuistCoreTesting/Utils/MockPrinter.swift 88.67% <0%> (-1.65%) ⬇️
Sources/TuistCore/Utils/Printer.swift 2.17% <0%> (+0.44%) ⬆️
...uistGeneratorTests/Models/CoreDataModelTests.swift 0% <0%> (ø) ⬆️
...ests/TuistGeneratorTests/Models/HeadersTests.swift 0% <0%> (ø) ⬆️
...torTests/Generator/DerivedFileGeneratorTests.swift 100% <100%> (ø) ⬆️
...sts/TuistKitTests/Commands/GraphCommandTests.swift 97.5% <100%> (ø) ⬆️
Tests/TuistKitTests/Models/Up/UpTests.swift 100% <100%> (ø) ⬆️
... and 53 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3bb1e3c...7f262ec. Read the comment docs.

@pepibumur pepibumur force-pushed the enable-colors branch from ae6c8f2 to 6a21064 Sep 16, 2019
Copy link
Contributor

marciniwanicki left a comment

Left few minor comments but overall looks really nice! 👌

Sources/TuistCore/Utils/Environment.swift Outdated Show resolved Hide resolved
Sources/TuistCore/Utils/Environment.swift Outdated Show resolved Hide resolved
Sources/TuistCore/Utils/Environment.swift Outdated Show resolved Hide resolved
Sources/TuistEnvKit/Commands/CommandRunner.swift Outdated Show resolved Hide resolved
Sources/TuistCore/Utils/Printer.swift Show resolved Hide resolved
@pepibumur pepibumur added this to Review in progress in Next version Sep 18, 2019
Next version automation moved this from Review in progress to Reviewer approved Sep 18, 2019
Copy link
Contributor

marciniwanicki left a comment

All good👌

@pepibumur pepibumur merged commit 54355b4 into master Sep 18, 2019
9 of 10 checks passed
9 of 10 checks passed
Build
Details
ci/circleci: cli Your tests failed on CircleCI
Details
Header rules No header rules processed
Details
Pages changed 19 new files uploaded
Details
Mixed content No mixed content detected
Details
Redirect rules 1 redirect rule processed
Details
ci/circleci: galaxy Your tests passed on CircleCI!
Details
ci/circleci: install Your tests passed on CircleCI!
Details
danger/danger ⚠️ 1 Warning. Don't worry, everything is fixable.
Details
deploy/netlify Deploy preview ready!
Details
Next version automation moved this from Reviewer approved to Done Sep 18, 2019
@pepibumur pepibumur deleted the enable-colors branch Sep 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Next version
  
Done
3 participants
You can’t perform that action at this time.