angularsen / UnitsNet

Makes life working with units of measurement just a little bit better.
https://www.nuget.org/packages/UnitsNet/
MIT No Attribution
2.2k stars 380 forks source link

Use official Jetbrains ReSharper annotations nuget #62

Closed oysteinkrog closed 9 years ago

oysteinkrog commented 9 years ago

There is now an official nuget for the annotations (with pcl support etc). It even has the option of embedding metadata so that users of units.net can get the benefits as well. https://www.nuget.org/packages/JetBrains.Annotations/

angularsen commented 9 years ago

The R# 9 templates has the same thing. However I've seen problems embedding the metadata in the binary. When two nugets do the same and use the same JetBrains.Annotations namespace you get a nasty conflict. It seems using external annotations via .xml file is the recommended approach for nugets, but it's quite tedious to setup.

oysteinkrog commented 9 years ago

I tried to reproduce the problem you mentioned but I can't seem to do it. As long as all projects involved use the official nuget/assemblies things work just fine? The official nuget ID is "JetBrains.Annotations".

angularsen commented 9 years ago

If you use the annotation nuget, you are probably not including the annotations in your compiled binary?

With R#9 templates I had to manually remove the compile condition attribute to make NotNull etc visible and effective to the user of the nuget. If two nugets did the same I got trouble. A R# doc said external annotations was the solution for that.

oysteinkrog commented 9 years ago

No I am.

Here is my test setup: I made a PCL class library with some simple types/methods annotated with NotNull etc. I made an additional normal class library, same kind of contents as above.

In both libraries I use the official jetbrains nuget and enabled metadata embedding (added JETBRAINS_ANNOTATIONS compilation symbol).

Then, I reference both projects in a third (wpf) project.

  1. Using the types from both class libraries is not a problem.
  2. Trying to use NotNull in this wpf project makes jetbrains suggest referencing only the official jetbrains attribute assembly, though it does suggest both the pcl and the non-pcl version, which I suppose is only reasonable as both are in fact valid options.

The JetBrains annotation types are never included in either of the class libraries, only the metadata (i.e. type1.method1 has a JetBrains.Annotation.NotNullAttribute attribute). As far as I can tell this is exactly what we want.

angularsen commented 9 years ago

If you can reference the PCL version of the annotations, doesn't that mean you have in fact embedded the annotation attribute types in the PCL binary, and not just the metadata as you say? I'm not even sure what metadata is exactly in this context. Is it some sort of IL type description without the actual implementation?

If I understand correctly, if you have 10 nugets, all embedding the R# annotations like this. Does that mean you will get 10 suggestions on how to import the NotNull attribute type?

oysteinkrog commented 9 years ago

The suggestion is for the nuget PCL assembly and the nuget "normal" assembly, not my own class libraries. R# probably looks at the assemblies that other projects in the solution is referencing and suggests based on that, the suggestion only appears if the projects are in the same solution.

I did some more testing: By making the WPF project in a separate solution and referencing the class library as an assembly instead I see the following behavior:

  1. If the class library assembly has the jetbrains.annotations assembly in the same folder, the wpf test project solution will offer to reference it if I try to use NotNull, and copy it when compiling.
  2. If I copy the compiled class library assembly somewhere else and reference it, no suggestion is offered, but when I decompile it I can still see the annotations. Compiling works despite the jetbrains assembly not being present.
  3. If I reference this class library assembly and add the jetbrains nuget I only get a suggestion for the nuget type, not the type metadata that is embedded in the class library. R# warns about notnull etc when using methods in the class library.

Dotpeek shows that the class library has a reference to the jetbrains assembly, and the attribute types are not embedded, just referenced. Any projects that use the class library do not need to reference the jetbrains annotations assembly, and the resulting exe runs fine without the jetbrains annotations assembly file present.

I feel reasonably certain this works just like the nuget advertises. I want to start using this in SQlite.Net, but it is important that users are not bothered by it, and that probably means that the nuget should not depend on the jetbrains nuget.. but that seems to work perfectly.

Please test this yourself if possible so we can be 100% sure. I think being able to provide notnoull/canbenull annotions to users is really really nice and worth the effort.

angularsen commented 9 years ago

Absolutely. I'll try to check some behavior in UnitsNet soon. Do you know if JetBrains recommend using the nuget over the templates, did they mention anything about that?

oysteinkrog commented 9 years ago

I don't really know anything about the templates, I just came across the nuget recently. The nuget is fairly new, as I looked really closely for an official one a while back and could not find it. https://twitter.com/controlflow/status/540269630867402752 They are using/exploiting the [Conditional] attribute to achieve the above/wanted behavior:

Alexander Shvedov ‏@controlflow Dec 3

@mstrobel yep, attribute class itself. If it's used only in attribute positions, it will be erased (whole binary dependency will be erased)
angularsen commented 9 years ago

Right. To my knowledge the templates and the nuget behavior seem identical, using the same conditional attribute stuff. It probably just comes down to preference about using nuget or adding the .cs file to each project. As for keeping updated, nuget is probably better.