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

#1254 Support VB.NET's case-insensitive assembly attributes #1255

Merged
merged 1 commit into from
Jun 15, 2016
Merged

#1254 Support VB.NET's case-insensitive assembly attributes #1255

merged 1 commit into from
Jun 15, 2016

Conversation

forgetaboutit
Copy link

Use case insensitive regex matching when looking for assembly attributes in .vb assembly info files.

Fixes issue #1254.

let private regexAttrOptionsCpp = Option.None
// VB.NET is case-insensitive
let private regexAttrOptionsVb = Option.Some (RegexOptions.IgnoreCase)

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need these.

@forgetaboutit
Copy link
Author

I fixed the issues you pointed out and updated the commit accordingly.

@inosik
Copy link
Contributor

inosik commented Jun 2, 2016

👍

Maybe you should also extend the specs here?

@forgetaboutit
Copy link
Author

forgetaboutit commented Jun 2, 2016

Digging into the code a bit more, I discovered the UpdateAttributes function, which is also exhibits the same problem. I will fix this as well and extend the specs accordingly. Please wait with the merge until I'm done.

Edit: What's the policy on squashing? Should I squash my commits when I'm done fixing?

@forgetaboutit
Copy link
Author

The new commit addresses both issues in GetAttributes and UpdateAttributes and extends the specs accordingly. I can think of nothing else that would prevent merging the fixes, but I'd be happy to fix any issues.

@forki
Copy link
Member

forki commented Jun 15, 2016

Awesome. thanks for taking care of this

@forki forki merged commit 2d84905 into fsprojects:master Jun 15, 2016
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