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

Adding option .directory for scaffolding templates #2985

Merged
merged 11 commits into from May 25, 2021

Conversation

santi-d
Copy link

@santi-d santi-d commented May 19, 2021

Short description πŸ“

Adding the possibility to copy folders when scaffolding process is made

Checklist βœ…

  • The code architecture and patterns are consistent with the rest of the codebase.
  • The changes have been tested following the documented guidelines.
  • The CHANGELOG.md has been updated to reflect the changes. In case of a breaking change, it's been flagged as such.
  • In case the PR introduces changes that affect users, the documentation has been updated.

@santi-d santi-d changed the title [WIP] Adding option for scaffolding files Adding option .directory for scaffolding templates May 20, 2021
@natanrolnik
Copy link
Collaborator

@santi-d we should document these additions here as well

@santi-d
Copy link
Author

santi-d commented May 21, 2021

@santi-d we should document these additions here as well

Was modified the file commented and also modified unit test that refers to scaffolding.

Copy link
Contributor

@pepicrft pepicrft left a comment

Choose a reason for hiding this comment

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

Thanks a lot for adding this improvement @santi-d. One more thing that it'd be great to add to this PR is extending this fixture to include a directory, and adding an extra step to the acceptance test that uses that feature, to ensure that the directory is copied as expected. This will prevent future regressions in your implementation. You can read more about acceptance tests here

@pepicrft
Copy link
Contributor

@all-contributors add @santi-d for code

@allcontributors
Copy link
Contributor

@pepibumur

I've put up a pull request to add @santi-d! πŸŽ‰

CHANGELOG.md Outdated Show resolved Hide resolved
Santiago A. Delgado and others added 3 commits May 21, 2021 09:26
@santi-d
Copy link
Author

santi-d commented May 21, 2021

Thanks a lot for adding this improvement @santi-d. One more thing that it'd be great to add to this PR is extending this fixture to include a directory, and adding an extra step to the acceptance test that uses that feature, to ensure that the directory is copied as expected. This will prevent future regressions in your implementation. You can read more about acceptance tests here

@pepibumur was modified fixture adding new template case with copy folder tree and acceptance testing was increased with this approach. thanks

Copy link
Member

@fortmarek fortmarek left a comment

Choose a reason for hiding this comment

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

This is looking good πŸ–€ just some minor comments πŸ™‚

/// - path: Path where will be copied the folder
/// - sourcePath: Path of folder which will be copied
/// - Returns: `Template.File` that is `.file`
static func directory(path: String, sourcePath: Path) -> Template.File {
Copy link
Member

Choose a reason for hiding this comment

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

the naming of Template.File does not really make sense anymore. Maybe Template.Item? I am open to other suggestions as well

Copy link
Author

@santi-d santi-d May 22, 2021

Choose a reason for hiding this comment

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

do you think is ok change also the signatures from files to items too (in all app)??
eg:

        let template = Template(
            description: "",
            attributes: [
                .required("name"),
                .optional("aName", default: "defaultName"),
                .optional("bName", default: ""),
            ],
            items: [
                .string(path: "static.swift", contents: "content"),
                .file(path: "generated.swift", templatePath: "generate.swift"),
                .directory(path: "destinationFolder", sourcePath: "sourceFolder"),
            ]
        )

Copy link
Member

Choose a reason for hiding this comment

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

yes, but I'd also keep the current init and mark it deprecated, so this does not end up a breaking change.

Copy link
Author

Choose a reason for hiding this comment

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

Added new init and the tag for deprecation.

@@ -54,4 +58,6 @@ Since platform is an optional argument, we can also call the command without the

If `.string` and `.files` don't provide enough flexibility, you can leverage the [Stencil](https://github.com/stencilproject/Stencil) templating language via the `.file` case. Besides that, you can also use additional filters defined [here](https://github.com/SwiftGen/StencilSwiftKit#filters)

Additionally is given `.directory` which gives the possibility to copy entire folders from one path inside another.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Additionally is given `.directory` which gives the possibility to copy entire folders from one path inside another.
You can also use `.directory` which gives the possibility to copy entire folders to a given path.

Copy link
Author

Choose a reason for hiding this comment

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

Changed, thanks :)

@@ -107,11 +107,19 @@ public final class TemplateGenerator: TemplateGenerating {
} else {
renderedContents = fileContents
}
case let .directory(path):
Copy link
Member

Choose a reason for hiding this comment

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

I'd test this in TemplateGeneratorTests

Copy link
Author

Choose a reason for hiding this comment

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

Added and tested :)

@@ -107,11 +107,19 @@ public final class TemplateGenerator: TemplateGenerating {
} else {
renderedContents = fileContents
}
case let .directory(path):
let destinationContainerPath = destinationPath.appending(components: [$0.path.pathString, path.basename])
Copy link
Member

Choose a reason for hiding this comment

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

I'd rename destinationContainerPath to destinationDirectoryPath.

Also nit: you can leverage the variadic arguments for appending() and rewrite .appending(components: [x, y]) to .appending(components: x, y)

Copy link
Author

Choose a reason for hiding this comment

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

cool, thanks

@pepicrft
Copy link
Contributor

add @santi-d for code

Copy link
Member

@fortmarek fortmarek left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this contribution! LGTM once the pipeline is green βœ…

@fortmarek fortmarek merged commit 74fbf01 into tuist:main May 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants