ironfede / openmcdf

Microsoft Compound File .net component - pure C# - netstandard 2.0
Mozilla Public License 2.0
295 stars 72 forks source link

What's the situation with OpenMcdf.Extensions calling internal methods in OpenMcdf? #110

Closed Numpsy closed 4 months ago

Numpsy commented 4 months ago

As it stands, OpenMcdf.Extensions is calling these two internal methods in the main lib via an InternalsVisibleTo attribute:

https://github.com/ironfede/openmcdf/blob/7f287e69691b6fa2b6b1ddbdffe3495286a96c34/sources/OpenMcdf/CFStream.cs#L205 https://github.com/ironfede/openmcdf/blob/7f287e69691b6fa2b6b1ddbdffe3495286a96c34/sources/OpenMcdf/CFStream.cs#L78

Does it need to be doing that? (just wondering, as it makes it harder to test changes in custom builds, as nothing other than OpenMcdf.Extensions.dll or the unit test libraries can call the required functions)

ironfede commented 4 months ago

@Numpsy why do you think that this makes tests harder? When originally implemented, those functions looks too specific and with a subtle meaning of the offset parameter to make them public even if they could be useful for some extension. This is the motivation for internal access modifier. I'm not saying that this thought is absolutely correct but could you provide an example of problems caused by [internal] modifier? Thank you!

Numpsy commented 4 months ago

My original reason for looking was that I'd made a custom build with all my WIP changes for custom properties to try to plug it into another test suite and had tried to rename it to make clear it was different from the official build, but if there's a good reason for the functions to be internal then this isn't enough of a reason for a change given that I can do test builds of the whole thing If I need to.