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

Workspace Improvements #298

Merged
merged 17 commits into from Mar 23, 2019

Conversation

Projects
None yet
3 participants
@kwridan
Copy link
Contributor

kwridan commented Mar 21, 2019

Resolves #258

Short description πŸ“

Workspace manifests allow users to customize which project are included in their workspace however doesn't yet offer the ability to add arbitrary files and folder references (to support included things like Documentation and README files).

Solution πŸ“¦

Extend the workspace manifest to allow specifying additionalFiles

e.g.

import ProjectDescription

let workspace = Workspace(name: "Workspace",
                          projects: [
                              "App", 
                              "Frameworks/FrameworkA", 
                              "Frameworks/FrameworkB", 
                            ],
                            additionalFiles: [
                                "Documentation/**",
                                .folderReference(path: "Website")
                            ])

As an added bonus of these changes, we can add the ability to specify glob patterns for projects to make it easier to include new projects without needing to constantly update the workspace manifest.

e.g.

import ProjectDescription

let workspace = Workspace(name: "Workspace",
                          projects: [
                              "App", 
                              "Frameworks/**", 
                            ],
                            additionalFiles: [
                                "Documentation/**",
                                .folderReference(path: "Website")
                            ])

Implementation πŸ‘©β€πŸ’»πŸ‘¨β€πŸ’»

  • Update workspace manifests
  • Parse workspace manifests > models
  • Add workspace structure generator
  • Integrate workspace structure
  • Include fixture
  • Update documentation

Test Plan βš’

  • Run tuist generate on the following fixtures:
    • fixture/ios_app_with_custom_workspace

custom-workspace

  • fixture/ios_app_with_custom_workspace/App

custom-workspace-app

  • fixture/ios_app_with_setup

setup

  • fixture/ios_app_with_frameworks

frameworks

Verify the generated workspace structure

Notes πŸ“

All this work is based on group effort and iteration from #262 - thanks @ollieatkinson, @pepibumur! πŸ‘

@kwridan kwridan requested a review from tuist/core Mar 21, 2019

@ollieatkinson
Copy link
Member

ollieatkinson left a comment

🏌️

Nice work - great to see this finished off with a fantastic solution.


let workspace = Workspace(name: project.name,
projects: graph.projects.map(\.path),
additionalFiles: workspaceFiles.map { .file(path: $0) })

This comment has been minimized.

Copy link
@ollieatkinson

ollieatkinson Mar 21, 2019

Member

This is equivalent to:

workspaceFiles.map(Workspace.Element.file) // assuming .file is an enum case inside of Workspace.Element

What do you think?

let files = fileHandler.glob(path, glob: string)

if files.isEmpty {
printer.print(warning: "No files found at: \(string)")

This comment has been minimized.

Copy link
@ollieatkinson

ollieatkinson Mar 21, 2019

Member

πŸ‘ Nice catch - but should these warnings live inside of the linter?

This comment has been minimized.

Copy link
@kwridan

kwridan Mar 21, 2019

Author Contributor

good point, will create a WorkspaceLinter

This comment has been minimized.

Copy link
@kwridan

kwridan Mar 21, 2019

Author Contributor

ouch, pushing this to the linter has one small snag πŸ˜”- at the point we reach the linter most of the useful information is lost e.g. the individual glob patterns that are invalid or don't yield any results

This comment has been minimized.

Copy link
@ollieatkinson

ollieatkinson Mar 21, 2019

Member

On one hand it would be nice to move this to the linter, but if it does not have the information maybe it's best to leave it here.

.filter(fileHandler.isFolder)
.filter {
manifestLoader.manifests(at: $0).contains(.project)
}

This comment has been minimized.

Copy link
@ollieatkinson

ollieatkinson Mar 21, 2019

Member

Depending on how much we care about optimisation (I haven't profiled this, so I don't know) we could flatten the recursion by turning these into lazy arrays. Same goes for the other functions defined here.

This comment has been minimized.

Copy link
@ollieatkinson

ollieatkinson Mar 21, 2019

Member

e.g. Replace usages of

fileHandler.glob(path, glob: string)

-->

fileHandler.glob(path, glob: string).lazy

Then any further mapping will also benefit.

Show resolved Hide resolved Sources/TuistKit/Generator/WorkspaceGenerator.swift Outdated
Show resolved Hide resolved Sources/TuistKit/Generator/WorkspaceStructureGenerator.swift Outdated
.file("/path/to/workspace/Pods.xcodeproj"),
.file("/path/to/workspace/Testing.playground"),
])
}

This comment has been minimized.

Copy link
@ollieatkinson

ollieatkinson Mar 21, 2019

Member

πŸ‘ These tests read really well

}

static func project(_ path: AbsolutePath) -> WorkspaceStructure.Element {
return .project(path: path)

This comment has been minimized.

Copy link
@ollieatkinson

ollieatkinson Mar 21, 2019

Member

what are the point of these functions?

This comment has been minimized.

Copy link
@kwridan

kwridan Mar 21, 2019

Author Contributor

purely to minify the test results comparisons - maybe overkill :/

e.g.

XCTAssertEqual(structure.contents, [
            .group(name: "Modules", path: "/path/to/workspace/Modules", contents: [
                .project(path: "/path/to/workspace/Modules/A"),
                .project(path: "/path/to/workspace/Modules/B"),
                .group(name: "Sub", path: "/path/to/workspace/Modules/Sub", contents: [
                    .project(path: "/path/to/workspace/Modules/Sub/C"),
                    .project(path: "/path/to/workspace/Modules/Sub/D"),
                ]),
            ]),
        ])

vs

XCTAssertEqual(structure.contents, [
            .group("Modules", "/path/to/workspace/Modules", [
                .project("/path/to/workspace/Modules/A"),
                .project("/path/to/workspace/Modules/B"),
                .group("Sub", "/path/to/workspace/Modules/Sub", [
                    .project("/path/to/workspace/Modules/Sub/C"),
                    .project("/path/to/workspace/Modules/Sub/D"),
                ]),
            ]),
        ])
// func testHello() {
// let sut = AppDelegate()
//
// XCTAssertEqual("AppDelegate.hello()", sut.hello())

This comment has been minimized.

Copy link
@ollieatkinson

ollieatkinson Mar 21, 2019

Member

We really need to fix this ... just flagging, not for this P/R

kwridan added some commits Mar 21, 2019

applying feedback suggestions:
- Workspace element mapping updates
- Leveraging lazy collections while mapping / filtering projects
@kwridan

This comment has been minimized.

Copy link
Contributor Author

kwridan commented Mar 21, 2019

thanks for the review! πŸ‘

@xcoderobot

This comment has been minimized.

Copy link

xcoderobot commented Mar 21, 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 Mar 21, 2019

Codecov Report

Merging #298 into master will increase coverage by 0.72%.
The diff coverage is 92.18%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #298      +/-   ##
==========================================
+ Coverage    88.4%   89.12%   +0.72%     
==========================================
  Files         271      276       +5     
  Lines        9552    10412     +860     
==========================================
+ Hits         8444     9280     +836     
- Misses       1108     1132      +24
Impacted Files Coverage Ξ”
...uistKitTests/Models/TestData/Target+TestData.swift 100% <ΓΈ> (ΓΈ) ⬆️
Sources/TuistKit/Graph/GraphLoader.swift 0% <0%> (ΓΈ) ⬆️
...ts/TuistKitTests/Graph/Mocks/MockGraphLoader.swift 100% <100%> (+100%) ⬆️
...Tests/Generator/Mocks/MockWorkspaceGenerator.swift 100% <100%> (+100%) ⬆️
Sources/TuistKit/Graph/Graph.swift 89.54% <100%> (-0.53%) ⬇️
...oreTests/Extensions/AbsolutePath+ExtrasTests.swift 100% <100%> (ΓΈ)
...ces/TuistCore/Extensions/AbsolutePath+Extras.swift 100% <100%> (ΓΈ) ⬆️
Tests/TuistKitTests/Generator/GeneratorTests.swift 100% <100%> (ΓΈ)
...rces/TuistKit/Generator/GeneratorModelLoader.swift 90.72% <100%> (+6.51%) ⬆️
.../TuistKitTests/Generator/Mocks/MockGenerator.swift 100% <100%> (ΓΈ) ⬆️
... and 24 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 2153c7d...8e879d5. Read the comment docs.

kwridan added some commits Mar 21, 2019

Merge branch 'master' into feature/workspace-additional-files
# Conflicts:
#	CHANGELOG.md
#	fixtures/README.md
@ollieatkinson

This comment has been minimized.

Copy link
Member

ollieatkinson commented Mar 22, 2019

Fixes #187

@ollieatkinson

This comment has been minimized.

Copy link
Member

ollieatkinson commented on 337cb12 Mar 23, 2019

Looks good πŸ•Ά

@kwridan kwridan merged commit fe44584 into master Mar 23, 2019

4 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
codecov/patch 92.18% of diff hit (target 88.4%)
Details
codecov/project 89.12% (+0.72%) compared to 2153c7d
Details
danger/danger ⚠️ 1 Warning. Don't worry, everything is fixable.
Details

@kwridan kwridan deleted the feature/workspace-additional-files branch Mar 23, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.