ironfede / openmcdf

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

Closes ironfede/openmcdf#19 .net standard support #20

Closed michaelp closed 6 years ago

michaelp commented 6 years ago

This commit fixes a few small issues that would obscure the changes required to support for .net standard

michaelp commented 6 years ago

@ironfede please review. This pull request is a minor fix, to prepare the ground for the follow-up changes

michaelp commented 6 years ago

@ironfede , I have a bit of an issue with deployment of the test files to the output directory of the tests To sum it up:

Proposed solution:

What do you think?

ironfede commented 6 years ago

It seems a clean solution. I think it's ok. P.S. Sorry for delayed responses but I'm not full time on this project

SimonCropp commented 6 years ago

@ironfede i was contemplating another PR. but it might conflict with this one. could this be merged so i could base my changes on this?

ironfede commented 6 years ago

Please Michael, let me know if last commit/merge is ok. Thank you.

Best Regards, Federico

michaelp commented 6 years ago

Federico ( @ironfede ) I checked locally, everything looks ok. Tests are "green". Thanks for merging us. A few points:

ironfede commented 6 years ago

Thank you for advices Michael, I'm going to build nuget package. I would like to have your opinion on some points: 1) Do you think I should keep major/minor constant (->2.1.7.xxx) for this release? 2) Where should I open a ticket? It should be a new issue? 3) I think that you can open a new issue (I will mark it as enanchement) to discuss about performance so we have a reference in order to future PRs and OpenMcdf project

altso commented 6 years ago

My two cents on version number. Changing the lib from netframework to netstandard could be a breaking change for consumers as they need binding redirects for all the libs in the standard. These redirects are handled automatically if a project uses PackageReference style nuget dependencies or in some other cases. Otherwise, binding redirects must be specified manually. In your case you're adding netstandard2.0 target in addition to net40, so all the existing consumers on .NET Framework will continue using net40 version. Consumers who target netcoreapp and use existing OpenMCDF for net40 (it's possible, you get a suppressable warning) will switch to netstandard version after upgrade. However, these projects already have tooling to generate binding redirects at compile time. I could be wrong on any or all of the statements above, but it seems to me that adding netstandard2.0 is a non breaking change, so only the minor version should be bumped. Changing the major number is a safe way of doing it though.

ironfede commented 6 years ago

Thanks for your advice @altso. So do you think I should switch to 2.2.xx release number (major unchanged, minor +1) ?

michaelp commented 6 years ago

@ironfede @altso ,

1.Ticket == create a standard issue in this repo and assign to me

  1. The openmcdf is now multi-targeted it means the same nuget package is targeting both .net framework and .net standard. So frankly, I am not sure that @altso had in mind. I expect this to be nonbreaking change.
  2. Since this change in non-breaking, I would recommend to follow the practice of minor version change and to keep the prefix. (I am not sure what @ironfede has established as a practice)
  3. To deal with the performance I need as much input as possible - i.e. the best help would be to have a well-defined set of use-cases.

P.s. I wish it was possible to improve this messaging interaction since currently, i need to pull for updates and I tend to miss the important messages directed to me. This is why my responses are delayed.

michaelp commented 6 years ago

Just for the sake of the doubt :
<TargetFrameworks>netstandard2.0;net40</TargetFrameworks> see the csproj

ironfede commented 6 years ago

@michaelp , waiting for your response to add ticket. Do you have any suggestion to improve message flow? I describe OpenMcdf numbering scheme M.m.r.b : next release will be 2.2.0.x in order to highlight new platform support. I think that Major should be changed ONLY for breaking changes (API refactoring, incompatible feature introduction, new file format support specs). Minor number mark new, compatible features, very high priority bug fixes, important features addition. Revision number mark bug fixes or small impact feature enhancements. Build number marked little or no changes in solution files with no impact on features of software and has been used, until now, as a simple signature of software release (auto generated).

altso commented 6 years ago

I agree with @michaelp. That's ok to increase minor version only.