Add shared instance to the Printer #483

pepicrft · 2019-08-19T18:54:37Z

Resolves #351

Short description 📝

Our current approach of using utilities like the FileHandler, System, or Printer, is cumbersome and requires a lot of boilerplate code and passing instances around.

Solution 📦

I added a shared static instance to the Printer.

import TuistCore

Printer.shared.print("whatever")

Any class in the project can access the shared instance of Printer easily.
That removes the need of having to inject the dependencies through the constructor and define instance variables to hold references to them.

When it comes to testing, I added a convenience static method that can be used from a test case to replace all utilities with their mocks. Here's an example of how the tests look:

import XCTestCase

class MyTest: XCTestCase {

  override func setUp() {
    mockEnvironment()
  }

}

Moreover, I added an extension to XCTestCase with some XCT assertion functions that can be used with context:

XCTAssertPrinterOutputContains("Project generated successfully")
XCTAssertPrinterErrorContains("The generation of the project failed")

…Context.shared

ollieatkinson

Thanks for doing this work.

It might be good to consider what other refactorings we might want to do in follow up pull requests for similar objects.

e.g.

FileHander
System
ResourceLocator
...

ollieatkinson · 2019-08-21T11:54:38Z

Sources/TuistCoreTesting/Utils/MockContext.swift

+    /// Mocks the shared context instance and returns the mock.
+    ///
+    /// - Returns: Mocked context.
+    static func mockSharedContext() -> MockContext {


It would be good to set this up automatically using XCTestObservation (see https://developer.apple.com/documentation/xctest/xctestobservation)

That's a good idea 😛

ollieatkinson · 2019-08-21T11:58:13Z

Sources/TuistCore/Utils/Context.swift

+    var deprecator: Deprecating { get }
+}
+
+public final class Context: Contexting {


How about just exposing public var printer: Printing and public var deprecator: Deprecating as top level variables. I was trying to understand what the benefit of grouping these in a single object is.

Context.shared.printer.print(deprecation: message)

printer.print(deprecation: message)

That's a good point. I can remove one level of indirection there. Thanks for the suggestion @ollieatkinson

pepicrft · 2019-08-22T22:37:45Z

@ollieatkinson I got rid of the Context class. The printer can now be accessed with Printer.shared. I looked into using XCTestObservation but it's not feasible because it requires adding an entry to the Info.plist, something that the Swift Package Manager doesn't allow.

What I did instead was extending the XCTestCase and add two methods, mockEnvironment() and mockPrinter(). They can be called explicitly from the tests when developers want to mock the environment.

pepicrft · 2019-08-22T22:37:57Z

@kwridan / @marciniwanicki could you have a look at this PR?

tuistbot · 2019-08-22T22:41:11Z

	2 Warnings
⚠️	Please include a CHANGELOG entry. You can find it at CHANGELOG.md.
⚠️	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.

SwiftLint found issues

Warnings

File	Line	Reason
MockPrinter.swift	56	SwiftLint rule 'force_try' did not trigger a violation in the disabled region. Please remove the disable command. (superfluous_disable_command)
MockPrinter.swift	61	SwiftLint rule 'force_try' did not trigger a violation in the disabled region. Please remove the disable command. (superfluous_disable_command)
ProjectGenerator.swift	77	Function body should span 40 lines or less excluding comments and whitespace: currently spans 41 lines (function_body_length)
UpHomebrew.swift	39	Use `allSatisfy` instead (reduce_boolean)

Generated by 🚫 Danger

codecov · 2019-08-22T23:05:55Z

Codecov Report

Merging #483 into master will increase coverage by 0.22%.
The diff coverage is 91.66%.

@@            Coverage Diff             @@
##           master     #483      +/-   ##
==========================================
+ Coverage    92.4%   92.63%   +0.22%     
==========================================
  Files         348      347       -1     
  Lines       17809    17752      -57     
==========================================
- Hits        16457    16444      -13     
+ Misses       1352     1308      -44

Impacted Files	Coverage Δ
...ratorTests/Generator/WorkspaceGeneratorTests.swift	`92.37% <ø> (-0.07%)`	⬇️
Sources/TuistKit/Models/Up/UpCustom.swift	`21.62% <0%> (ø)`	⬆️
Sources/TuistKit/Commands/BuildCommand.swift	`0% <0%> (ø)`	⬆️
Sources/TuistEnvKit/Commands/VersionCommand.swift	`0% <0%> (ø)`	⬆️
Sources/TuistGenerator/Dot/DotGraphGenerator.swift	`0% <0%> (ø)`	⬆️
Sources/TuistKit/Models/Up/Up.swift	`73.68% <0%> (ø)`	⬆️
Sources/TuistKit/Commands/VersionCommand.swift	`0% <0%> (ø)`	⬆️
Sources/TuistKit/Commands/EmbedCommand.swift	`0% <0%> (ø)`	⬆️
Sources/TuistKit/Commands/FocusCommand.swift	`38.88% <0%> (+2.99%)`	⬆️
...stEnvKitTests/Commands/UninstallCommandTests.swift	`100% <100%> (ø)`	⬆️
... and 58 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 71a3ec0...83954a5. Read the comment docs.

codecov · 2019-08-22T23:05:55Z

Codecov Report

Merging #483 into master will increase coverage by 0.22%.
The diff coverage is 91.66%.

@@            Coverage Diff             @@
##           master     #483      +/-   ##
==========================================
+ Coverage    92.4%   92.63%   +0.22%     
==========================================
  Files         348      347       -1     
  Lines       17809    17752      -57     
==========================================
- Hits        16457    16444      -13     
+ Misses       1352     1308      -44

Impacted Files	Coverage Δ
...ratorTests/Generator/WorkspaceGeneratorTests.swift	`92.37% <ø> (-0.07%)`	⬇️
Sources/TuistKit/Models/Up/UpCustom.swift	`21.62% <0%> (ø)`	⬆️
Sources/TuistKit/Commands/BuildCommand.swift	`0% <0%> (ø)`	⬆️
Sources/TuistEnvKit/Commands/VersionCommand.swift	`0% <0%> (ø)`	⬆️
Sources/TuistGenerator/Dot/DotGraphGenerator.swift	`0% <0%> (ø)`	⬆️
Sources/TuistKit/Models/Up/Up.swift	`73.68% <0%> (ø)`	⬆️
Sources/TuistKit/Commands/VersionCommand.swift	`0% <0%> (ø)`	⬆️
Sources/TuistKit/Commands/EmbedCommand.swift	`0% <0%> (ø)`	⬆️
Sources/TuistKit/Commands/FocusCommand.swift	`38.88% <0%> (+2.99%)`	⬆️
...stEnvKitTests/Commands/UninstallCommandTests.swift	`100% <100%> (ø)`	⬆️
... and 58 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 71a3ec0...519c742. Read the comment docs.

ollieatkinson

Lots of similar changes, and running locally still shows the correct output! 👍

kwridan · 2019-08-23T12:37:25Z

Thanks for making the sweeping changes @pepibumur

Personally, I quite liked the previous approach of having explicit dependencies. I agree that passing them around everywhere was quite cumbersome - for that @marciniwanicki's suggestions would help address that not only for those lower level components but for some of the other bigger ones too.

I don't have a strong view on this, so let's give the changes here a go and revisit it when needed 🙂

The good thing here is we can still control this for testing and when using Tuist as a library 👍

Pedro Piñera added 5 commits August 19, 2019 16:07

Implement Context and update usages of the printer to use it through …

5354a93

…Context.shared

Fix tests

18a8e94

Add XCTAssertPrinterError and XCTAssertPrinterError

5bdb6c3

Rename the XCT methods

850fe50

Simplify API

406a857

pepicrft requested a review from a team August 19, 2019 18:54

pepicrft self-assigned this Aug 19, 2019

ollieatkinson reviewed Aug 21, 2019

View reviewed changes

pepicrft mentioned this pull request Aug 22, 2019

Dependency injection #351

Closed

Get rid of Context and provide a mockEnvironment() helper method

83954a5

Fix linting issues

519c742

pepicrft changed the title ~~Add context shared utility and move printer to it~~ Add shared instance for the Printer Aug 23, 2019

pepicrft changed the title ~~Add shared instance for the Printer~~ Add shared instance to the Printer Aug 23, 2019

pepicrft requested a review from a team August 23, 2019 10:33

ollieatkinson approved these changes Aug 23, 2019

View reviewed changes

pepicrft merged commit 27bb970 into master Aug 23, 2019

pepicrft deleted the context branch August 23, 2019 12:59

pepicrft mentioned this pull request Aug 23, 2019

FileHandler.shared #488

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add shared instance to the Printer #483

Add shared instance to the Printer #483

pepicrft commented Aug 19, 2019 •

edited

ollieatkinson left a comment

ollieatkinson Aug 21, 2019

pepicrft Aug 21, 2019

ollieatkinson Aug 21, 2019

pepicrft Aug 21, 2019

pepicrft commented Aug 22, 2019

pepicrft commented Aug 22, 2019

tuistbot commented Aug 22, 2019 •

edited

codecov bot commented Aug 22, 2019

codecov bot commented Aug 22, 2019 •

edited

ollieatkinson left a comment

kwridan commented Aug 23, 2019

Add shared instance to the Printer #483

Add shared instance to the Printer #483

Conversation

pepicrft commented Aug 19, 2019 • edited

Short description 📝

Solution 📦

ollieatkinson left a comment

Choose a reason for hiding this comment

ollieatkinson Aug 21, 2019

Choose a reason for hiding this comment

pepicrft Aug 21, 2019

Choose a reason for hiding this comment

ollieatkinson Aug 21, 2019

Choose a reason for hiding this comment

pepicrft Aug 21, 2019

Choose a reason for hiding this comment

pepicrft commented Aug 22, 2019

pepicrft commented Aug 22, 2019

tuistbot commented Aug 22, 2019 • edited

SwiftLint found issues

Warnings

codecov bot commented Aug 22, 2019

Codecov Report

codecov bot commented Aug 22, 2019 • edited

Codecov Report

ollieatkinson left a comment

Choose a reason for hiding this comment

kwridan commented Aug 23, 2019

pepicrft commented Aug 19, 2019 •

edited

tuistbot commented Aug 22, 2019 •

edited

codecov bot commented Aug 22, 2019 •

edited