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

Improve scaffold feature's default parameter type for stencil file #5845

Merged
merged 14 commits into from
Feb 6, 2024

Conversation

haeseoklee
Copy link
Contributor

Resolves #5462

Short description πŸ“

The scaffold template's optional parameter is improved to support a variety of types (including Dictionary, Int..) for stencil file. The expected effect is that stencil's renderTemplate method will be able to receive a custom context.
Additionally, after being able to pass various types of parameters in the source code, users can construct a more expressive template file.

After

let attribute: Template.Attribute = .optional("environment", default: "string value")
// or
let attribute: Template.Attribute = .optional("environment", default: ["key": "value", "key2": "value2"])
// or
let attribute: Template.Attribute = .optional("environment", default: 99999)
// or
let attribute: Template.Attribute = .optional("environment", default: true)

How to test the changes locally 🧐

Include a set of steps for the reviewer to test the changes locally (see the documentation for reference).

Contributor checklist βœ…

  • The code has been linted using run mise run lint:fix
  • The change is tested via unit testing or acceptance testing, or both
  • The title of the PR is formulated in a way that is usable as a changelog entry
  • In case the PR introduces changes that affect users, the documentation has been updated

Reviewer checklist βœ…

  • The code architecture and patterns are consistent with the rest of the codebase
  • Reviewer has checked that, if needed, the PR includes the label changelog:added, changelog:fixed, or changelog:changed, and the title is usable as a changelog entry

@haeseoklee haeseoklee force-pushed the issue#5462 branch 2 times, most recently from ec9afdd to 7b1d617 Compare January 26, 2024 15:54
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 PR πŸ‘

Might be also worth updating one of the scaffold acceptance tests to test this end-to-end.

The test failure looks unrelated and should be fixed by updating the branch.

@haeseoklee
Copy link
Contributor Author

haeseoklee commented Jan 30, 2024

@danyf90 @fortmarek
Thank you for the review.

While writing additional tests, a critical issue was discovered. Unlike Template.Attribute in TuistGraph, Template.Attribute in ProjectDescription must conform to the Codable protocol. Since AnyHashable cannot conform to Codable, we need to come up with another idea.

스크란샷 2024-01-31 α„‹α…©α„Œα…₯ᆫ 2 13 54

One idea that comes to mind is to provide users with an explicit interface. We can define possible types as an enum and use associated values to store the desired values.

For example, .optional("name", default: "platform") can be represented as follows: .optional("name", default: .string("platform")) .

In the same context, various types such as bool, integer, dictionary, etc., are also possible.

@haeseoklee
Copy link
Contributor Author

If the interface changes, it could be a breaking change.

@danieleformichelli
Copy link
Collaborator

I think we need something like PList.Value:

public indirect enum Value: Codable, Equatable {

@haeseoklee
Copy link
Contributor Author

@danyf90
I will give it a try by defining an enum.

@pepicrft pepicrft added the changelog:added PR will be listed in the Added section of CHANGELOG label Feb 5, 2024
@pepicrft
Copy link
Contributor

pepicrft commented Feb 5, 2024

@haeseoklee do you mind rebasing from the latest changes in main? We merged a branch with significant breaking changes and the work here is a bit behind. Let me know if you have questions when doing the rebase.

@haeseoklee
Copy link
Contributor Author

@pepicrft Thank you, I will try to do the rebase

@danieleformichelli danieleformichelli enabled auto-merge (squash) February 5, 2024 17:02
auto-merge was automatically disabled February 5, 2024 23:55

Head branch was pushed to by a user without write access

@haeseoklee
Copy link
Contributor Author

haeseoklee commented Feb 6, 2024

@danyf90

e4e69d0

I reverted this commit. There is an issue with TuistGraph.Template.Value not passing the tests. We can resolve this problem, but it seems overly complex.

@danieleformichelli
Copy link
Collaborator

Thank you, please avoid force pushing on the branch. We squash-commit anyway so it just makes review harder πŸ™

@pepicrft
Copy link
Contributor

pepicrft commented Feb 6, 2024

The failing tests are unrelated to your changes and have already been fixed in main. I'll go ahead with the merge.

@pepicrft pepicrft merged commit 0a681db into tuist:main Feb 6, 2024
8 of 9 checks passed
@haeseoklee
Copy link
Contributor Author

@danyf90
I will refrain from frequent rebasing in the future.
It was my first contribution, and I received a lot of help. Thank you very much.

@danieleformichelli
Copy link
Collaborator

@danyf90

I will refrain from frequent rebasing in the future.

It was my first contribution, and I received a lot of help. Thank you very much.

No worries! Thanks a lot for your contribution πŸ”₯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:added PR will be listed in the Added section of CHANGELOG
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extend scaffold to allow passing non-string parameteres to Stencil templates
4 participants