-
Notifications
You must be signed in to change notification settings - Fork 695
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
Fixes #3109. AOT support with .Net 8. #3638
Conversation
… the unit tests configurations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I may have broken something with my fixing of merge conflicts though!
Considerations to take an account when working with
using System.Text.Json.Serialization;
namespace Terminal.Gui;
[JsonConverter (typeof (JsonStringEnumConverter<MyEnum>))]
public enum MyEnum
{}
...
[JsonSerializable (typeof (MyEnum))]
[SerializableConfigurationProperty (Scope = typeof (MyScope))]
public static MyEnum DefaultMyEnum { get; set; } = MyEnum.{Wathever}; // Default is set in config.json
|
Could you add a unit test that verifies all these conditions are true for any enum defined in the libarary? |
If you know how to can run unit tests from a executable, of course. The problem is that the unit tests are tested with al the library on the |
I must not have made myself clear:
It should be possible to build a unit test that verifies that any newly added enums have those attributes. |
But these attributes are only needed to be added if they are used on
Only if they are used on |
…erialization issue when published file runs.
@tig I changed the
|
So you think it's better to let a developer have to get all the way to publishing the NaviteAot project to discover they missed following instructions than having a test in |
The user doesn't have to worry about that. Must be us, as maintainer ensuring the |
First, we have a bunch of unit tests that are for US. E.g. all the Disposed stuff. It has saved countless hours of OUR time. Second, I don't think it's that hard to make a unit test that does what I suggest. No, I'm not going to do it just to prove it. I guarantee, though, that at some point in the future one of us maintainers is going to forget to properly attribute an enum and the only way we'll find out is because someone ELSE tries to publish a Native AOT app. When that happens that will be OUR fault, for not taking the time to build as much testing in as we can. |
Well you can try to do yourself because it's beyond of my limits. |
I'm sorry you feel that way. It would be great if you would reconsider. |
I don't have Copilot but I'll try do it. |
I could have asked ChatGPT or any other of a dozen free dev AIs. |
Done @tig. Explanation:1. Assembly and Types Inspection:
2. Collecting Property Types:
3. Get Registered Types:
4. Assertions for Properties:
5. Identifying Classes with
This comprehensive approach ensures that all relevant types are checked and validated, maintaining the robustness and flexibility of the test. |
Ready to merge? |
Yes. Thanks. |
@tig I don't know if the test will not fail it another derived from type of |
Nah. That seems rare enough that we're probably ok until it becomes a problem. |
Sorry @tig al least I learn that is impossible to test from all JsonConverter<> derived classes because we need the types passed in the generic at runtime and not at compile time. So, DictionaryJsonConverter the T type will be only know at run time. |
Fixes
Proposed Changes/Todos
Pull Request checklist:
CTRL-K-D
to automatically reformat your files before committing.dotnet test
before commit///
style comments)