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.

Sign up for GitHub

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

Jump to bottom

Fixes for Assembly info updating #1959

Merged
merged 13 commits into from
May 24, 2018
Merged

Fixes for Assembly info updating #1959

merged 13 commits into from
May 24, 2018

Conversation

BlythMeister
Copy link
Contributor

No description provided.

@BlythMeister
Update paket bootstrapper
22bd58e
@BlythMeister
Fix outputting of AssemblyInfoFile
fd82c8b 
This should output the static value as this is quote escaped when required
@BlythMeister
Update reading of AssemblyInfoFile to use the typed values for known …
ec9e0ba 
…attributes

This means the correct namespaces are available rather than the default blank
@BlythMeister
When getting dependencies do not include any empty strings
1e8be82
@matthid
Copy link
Member

matthid commented May 22, 2018

Can you briefly explain what the issue was or what this change does? Can we test it?

@BlythMeister
Copy link
Contributor Author

Assembly info with string values are not in quotes when you write them out.
Therefore you get a compile error.

@BlythMeister
Copy link
Contributor Author

Also, when you get attributes, they don't have the namespaces as they are all empty

@matthid
Copy link
Member

matthid commented May 22, 2018

So with your change "Attribute.Name" is no longer quoted or what is the difference to StaticPropertyName?
If it should no longer be quoted we should change the constructors as well, correct?
Also as StringAttribute is calling StringAttributeEx it seems like the name is quoted twice...

I think I'm slowly getting parts of what the problem was, sorry ;)

@BlythMeister
Copy link
Contributor Author

Sorry, I don't think I'm explaining well 😞

The value of all attributes in the "name" is not quoted.
The static value of strings are quoted, but if bool is not quoted.
The static and normal names are actually the same, but other code used static name, so I made that consistent.

@BlythMeister
Copy link
Contributor Author

The second change relates to how the assembly info is read.
Since when doing a get on the attributes the namespace was always empty.
So the match now looks at the type/name of attribute and creates as a strongly typed version of the attribute so it has the namespace

@BlythMeister
Copy link
Contributor Author

But I also think your right, this module should have some tests over it, so I'll add some tomorrow 👍🏼

@BlythMeister
Copy link
Contributor Author

The only way I can see a double quote occuring is if someone was creating an attribute with a quoted value in the string.
I can update the code so this will not double quote too 🙂 (and add that as a test case)

@matthid
Copy link
Member

matthid commented May 22, 2018

The value of all attributes in the "name" is not quoted.
The static value of strings are quoted, but if bool is not quoted.
The static and normal names are actually the same, but other code used static name, so I made that consistent.

Thanks for the explanation. In particular we probably should also fix:

https://github.com/BlythMeister/FAKE/blob/87b1fcf2caae8ca2bbe6a93b5313dc162a8adbad/src/app/Fake.DotNet.AssemblyInfoFile/AssemblyInfoFile.fs#L79-L84

image

Maybe we should indeed make "Name" unquoted and the other one quoted. Seems a bit strange currently.

Thanks for taking care of this. Sorry was a bit slow there but the code is a bit strange :)

@matthid
Copy link
Member

matthid commented May 22, 2018

Ah now I get it. It doesn't quote twice but it first quotes the staticValue and later the value. Wow

@BlythMeister
Copy link
Contributor Author

BlythMeister commented May 22, 2018 •
edited
Loading

Oh wow...that's messed up. - more than I actually thought!
Ok, I'll add some tests and make this clearer to understand 😂

@BlythMeister
Copy link
Contributor Author

@matthid this should now be complete with tests for cs, fs and vb assembly info files

@BlythMeister
Copy link
Contributor Author

i also fixed the launch and task json files to run nicely in vscode

matthid
matthid reviewed May 23, 2018
View reviewed changes
Fake.sln Outdated
# Visual Studio 15
VisualStudioVersion = 15.0.27130.2026
MinimumVisualStudioVersion = 15.0.26124.0
Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "app", "app", "{7BFFAE76-DEE9-417A-A79B-6A6644C4553A}"
EndProject
Project("{6EC3EE1D-3C4E-46DD-8F32-0CC8E7565705}") = "Fake.Core.Context", "src/app/Fake.Core.Context/Fake.Core.Context.fsproj", "{D3D92ED7-C2B9-46D5-B611-A2CF0C30C8DB}"
Project("{6EC3EE1D-3C4E-46DD-8F32-0CC8E7565705}") = "Fake.Core.Context", "src\app\Fake.Core.Context\Fake.Core.Context.fsproj", "{D3D92ED7-C2B9-46D5-B611-A2CF0C30C8DB}"
Copy link
Member

Choose a reason for hiding this comment

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

Can you revert that and make sure to use /?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Drat, didn't mean to include that...stupid windows.

matthid
matthid reviewed May 23, 2018
View reviewed changes
src/app/Fake.DotNet.AssemblyInfoFile/AssemblyInfoFile.fs Outdated

let StringAttribute(name, value, inNamespace) =
let quotedValue = quote value
StringAttributeEx(name, value, inNamespace, name, quotedValue)
Attribute(name, quote quotedValue, inNamespace, name, typeof<string>.FullName, quotedValue)
Copy link
Member

Choose a reason for hiding this comment

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

quote quotedValue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Typo, will fix (because of quote, it's ok since it will never double quote, but best to fix it)

matthid
matthid reviewed May 23, 2018
View reviewed changes
src/test/Fake.Core.UnitTests/Fake.Core.Context.fs
@@ -19,7 +17,7 @@ let tests =
myC
try
Fake.Core.Context.forceFakeContext() |> ignore
Assert.Fail "Expected exception"
Tests.failtest "Expected exception"
Copy link
Member

Choose a reason for hiding this comment

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

Ah there it is, never found this API myself :/

@BlythMeister
Copy link
Contributor Author

@matthid all updated.
Also added a generic "create" function which calls the correct method based on the file extension

@BlythMeister
Copy link
Contributor Author

phew it builds!

@BlythMeister BlythMeister mentioned this pull request May 24, 2018
Merged
@matthid matthid changed the base branch from master to release/rc May 24, 2018 17:20
@matthid
Copy link
Member

matthid commented May 24, 2018

Thanks for taking care of this :)

@matthid matthid merged commit cabe318 into fsprojects:release/rc May 24, 2018
@BlythMeister
Copy link
Contributor Author

@matthid your more than welcome. Hope to be able to contribute more here and in paket 👍🏼

@matthid matthid mentioned this pull request May 27, 2018
Closed
@BlythMeister BlythMeister deleted the AssemblyInfo branch October 18, 2018 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthid matthid matthid left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@BlythMeister @matthid