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

Make ExplicitExports "true” by default (or at least make it default in the .dna template) #29

Closed
augustoproiete opened this issue Sep 5, 2015 · 5 comments

Comments

@augustoproiete
Copy link
Member

I think ExplicitExports should be true by default as IMO this is what most ExcelDna developers would want/expect.

I've been bitten by this before I knew this attribute existed, and leaked a few static classes as formulas in an add-in, which caused me some pain.

I realize this could break existing add-ins, but sometimes breaking changes are necessary - and if documented well, should not cause too many issues.

If introducing this change is not an option in the short-term, I'd suggest at least update the ExcelDna-Template.dna to declare ExplicitExports="true” there so that anyone creating a new add-in will get the expected behavior (and also makes this attribute more discoverable).

<DnaLibrary Name="%ProjectName% Add-In" RuntimeVersion="v4.0">
  <ExternalLibrary Path="%OutputFileName%" LoadFromBytes="true" Pack="true" ExplicitExports="true” />
</DnaLibrary>
@govert
Copy link
Member

govert commented Sep 5, 2015

ExplicitExports should definitely be 'false' by default and should be 'false' in the .dna template by default - this makes the VB situation friendlier, and is compatible with the past.

However, I'm happy to have the template in the NuGet package include both the ExplicitExport='false' tag, and a comment that describes the situation. So anything like this:

<DnaLibrary Name="%ProjectName% Add-In" RuntimeVersion="v4.0">
  <ExternalLibrary Path="%OutputFileName%" LoadFromBytes="true" Pack="true" ExplicitExports="false" />

<!-- ExplicitExport indicates whether exported function and macros need to be explicitly marked with [ExcelFunction(...)] and [ExcelCommand(...)] macros to be exported, or whether all public static methods with compatible signature are registered.
The behavior is as follows: .....

Read more here:....

-->
</DnaLibrary>

@augustoproiete
Copy link
Member Author

@govert: Could you expand on the "makes the VB situation friendlier" part? Is there a difference between C# and VB with regards to the exports of functions?

Adding the attribute to the .dna template, even if false by default, seems like a step in the right direction to me, so 👍.

I do, however, think would be nice to go a little bit further and - at least - declare it as true for newly created add-ins, and thus still staying compatible with existing add-ins.

@govert
Copy link
Member

govert commented Sep 5, 2015

I mean for somebody coming from VBA, it's pretty cool that the basic code can look like this

Public Module MyFunctions
    Function AddThem(v1, v2)
        AddThem = v1 + v2
    End Function
End Module

It's bad enough that you need the Public Module ... around it, but at least you don't need to know about attributes to get started.
I value that enough, together with the backward compatibility, to want to keep the default 'false'.

@augustoproiete
Copy link
Member Author

augustoproiete commented Sep 5, 2015

Got it.

I see your point re: simplicity, but I honestly question if that is "real code" vs "demo code".

I've never seen a real-world add-in that simple, where every function is self-contained and don't call other utility functions to get some reuse.

As soon as you get into something almost as simple:

Public Module MyFunctions
    Function Calculate(v1, calcType)
        denominator = GetDenominator(calcType)
        Calculate = v1 / denominator
    End Function

    Function GetDenominator(calcType)
        Case calcType = "x"
            GetDenominator = 10
        Case calcType = "y"
            GetDenominator = 20
        Default
            GetDenominator = 1
    End Function
End Module

You're in trouble... As GetDenominator now is exposed to the world and your public API surface is bigger than you wanted it to be - and by the time you notice it, it could be too late and people are already using GetDenominator in their spreadsheets.

Now imagine you have 50 methods that got exposed unintentionally, instead of one... 50 more methods to support / keep backwards compatibility, etc. than you originally planned for.

So in general, I think being explicit on what you make public to the user, leads to a better design.

Anyway, having the attribute in the template is still a good improvement.

@govert
Copy link
Member

govert commented Sep 29, 2015

Check-in 37326da adds the ExplicitExports="false" to the .dna file template.

@govert govert closed this as completed Sep 29, 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

No branches or pull requests

2 participants