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

Change SignToolHelper syntax to reflect common call syntax #1051

Merged
merged 1 commit into from
Dec 28, 2015
Merged

Change SignToolHelper syntax to reflect common call syntax #1051

merged 1 commit into from
Dec 28, 2015

Conversation

pdfforge
Copy link

Until now, the call syntax of the SignToolHelper would have been
something like this:

 let signParams = {
     DevCertificate = cert
     Certificate = None
     FilesToSign = !! (buildDir @@ "**/*.exe")
     TimeStampUrl = None
 }

 Sign "packages/signtool" signParams

This does not go well with piping file sequences as it is used all
over FAKE. After this change, SignTool can be called like this:

 let signParams = {
     DevCertificate = cert
     Certificate = None
     TimeStampUrl = None
 }

 !! (buildDir @@ "**/*.exe")
   |> Sign "packages/signtool" signParams

From my point of view, this is more the "FAKE" way of doing things.

This is a breaking change though, as it changes the sytax of this helper and changes the SignParams type.

Until now, the call syntax of the SignToolHelper would have been
something like this:

     let signParams = {
         DevCertificate = cert
         Certificate = None
         FilesToSign = !! (buildDir @@ "**/*.exe")
         TimeStampUrl = None
     }

     Sign "packages/signtool" signParams

This does not go well with piping file sequences as it is used all
over FAKE. After this change, SignTool can be called like this:

     let signParams = {
         DevCertificate = cert
         Certificate = None
         TimeStampUrl = None
     }

     !! (buildDir @@ "**/*.exe")
       |> Sign "packages/signtool" signParams

From my point of view, this is more the "FAKE" way of doing things.
@forki
Copy link
Member

forki commented Dec 21, 2015

yes this would have been better. But since it will break A LOT of builds I think it's not a good idea to change it.

@forki forki closed this Dec 21, 2015
@pdfforge
Copy link
Author

Sorry for bothering again with this issue.

After some investigation, it happens that the file has never been in the build before (not even in the initial commit b21b39e). So in my opinion, this can safely be merged, as it had not been included.

If you don't agree, that's ok of course ;-)

@forki forki reopened this Dec 28, 2015
forki added a commit that referenced this pull request Dec 28, 2015
Change SignToolHelper syntax to reflect common call syntax
@forki forki merged commit 21e87c4 into fsprojects:master Dec 28, 2015
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