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

Remove Sourcery version from header #1309

Merged

Conversation

dcacenabes
Copy link
Contributor

As discussed in issue #399 , including the Sourcery version in the generated files headers has side effects, like higher CI times due to unnecessary cache invalidation.

This PR removes any references to the version as part of the file headers

@art-divin
Copy link
Collaborator

@dcacenabes Hello 👋🏻

Thank you very and very much for being initiative and creating this PR so quick!

I wonder, can we have such a strategy to have a runtime flag, which would do what you did here?
I do not want to trigger such a huge diff for all consumers of Sourcery 2.1.9 due to this change, rather, we can provide this feature optionally via a flag, also adding a test covering this option.

What do you think?

@dcacenabes
Copy link
Contributor Author

@dcacenabes Hello 👋🏻

Thank you very and very much for being initiative and creating this PR so quick!

I wonder, can we have such a strategy to have a runtime flag, which would do what you did here? I do not want to trigger such a huge diff for all consumers of Sourcery 2.1.9 due to this change, rather, we can provide this feature optionally via a flag, also adding a test covering this option.

What do you think?

Makes total sense! It will take me a little bit longer, since I am not familiar with the codebase and I need to understand it a little bit better.

@dcacenabes
Copy link
Contributor Author

dcacenabes commented Mar 25, 2024

Just to make sure we're on the same page, you are suggesting to pass a command line option, something like force-parse or cacheBasePath ?

Edit: More along the lines of logBenchmarks and parseDocumentation

Flag("logBenchmarks", description: "Log time benchmark info"),
Flag("parseDocumentation", description: "Include documentation comments for all declarations."),
, which are flags

@art-divin
Copy link
Collaborator

Just to make sure we're on the same page, you are suggesting to pass a command line option, something like force-parse or cacheBasePath ?

Yes, that would be perfect.
To make things easier for you, ask all questions you have, sooner you'll ask, sooner I'll be able to accommodate you with an answer 🤝

Sourcery/Sourcery.swift Outdated Show resolved Hide resolved
@dcacenabes
Copy link
Contributor Author

Just to make sure we're on the same page, you are suggesting to pass a command line option, something like force-parse or cacheBasePath ?

Yes, that would be perfect. To make things easier for you, ask all questions you have, sooner you'll ask, sooner I'll be able to accommodate you with an answer 🤝

I pushed some changes.
I also added a test although not sure it is the best context to add it.
Happy to apply any feedback you have to the implementation.

@art-divin
Copy link
Collaborator

I also added a test although not sure it is the best context to add it.

Oh well, one can never be sure, there are 600 behavioural tests and one can only guess where to put the test. But once you write it, if it becomes too much of a hassle to put it into that specific Spec file, or everything around looks too "unrelated" - then you might want to consider other Spec files for its placement - my approach is like this to this day.

@art-divin
Copy link
Collaborator

I will merge this, and before the new release, will test with the automation if some regex needs to be adjusted in the scripting.

Thank you very much 🤝 @dcacenabes and welcome to the Contributor club 🥳 🔥

@art-divin art-divin merged commit 33be2ab into krzysztofzablocki:master Mar 26, 2024
2 checks passed
art-divin added a commit that referenced this pull request Mar 26, 2024
commit 33be2ab
Author: David Cacenabes <[email protected]>
Date:   Tue Mar 26 07:09:02 2024 +0100

    Remove Sourcery version from header (#1309)

    * Remove version from headers

    * Remove from generated files

    * Revert "Remove from generated files"

    This reverts commit 9a8c14e.

    * Revert "Remove version from headers"

    This reverts commit 93ddd54.

    * Add support to hide header via config

    * Remove redundant self

    * Make non-lazy
@art-divin art-divin added this to the 2.1.9 milestone Mar 26, 2024
art-divin added a commit that referenced this pull request Mar 26, 2024
* Update SwiftTemplate.swift

Enables a single point of output into STDOUT during codegen from swifttemplate

* Fixed failing tests

* Squashed commit of the following:

commit 33be2ab
Author: David Cacenabes <[email protected]>
Date:   Tue Mar 26 07:09:02 2024 +0100

    Remove Sourcery version from header (#1309)

    * Remove version from headers

    * Remove from generated files

    * Revert "Remove from generated files"

    This reverts commit 9a8c14e.

    * Revert "Remove version from headers"

    This reverts commit 93ddd54.

    * Add support to hide header via config

    * Remove redundant self

    * Make non-lazy
@dcacenabes
Copy link
Contributor Author

I will merge this, and before the new release, will test with the automation if some regex needs to be adjusted in the scripting.

Thank you very much 🤝 @dcacenabes and welcome to the Contributor club 🥳 🔥

Very happy to be a contributor! 🎉 Thanks to you for your support and fast response!

@visma-ios-if
Copy link

visma-ios-if commented May 1, 2024

@dcacenabes I know this Pr is closed for some time now. But I am not able to figure it out how can I actually make use of this new flag hideVersionHeader in my sorcery .yml file, note, I am using the lates Sorcery release 2.2.3 ( I am seeing in the header)
I tried to use it like this:

sources:
  - {path to my sources}
templates:
  - {path to stencil file template}
output:
  {output path}
hideVersionHeader: true

But it do not matter how many times I am trying to generate the file, the header comments including the version are in there.
Can you give an example on how to use it?

@art-divin
Copy link
Collaborator

Hello @visma-ios-if ,

thank you very much for raising this!

I've found out that this was not supported, and so I have added this feature in #1328. Please see PR's description for the format.

@dcacenabes
Copy link
Contributor Author

Thanks for taking care of this!

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.

3 participants