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

Objentry additions #95

Merged
merged 17 commits into from
May 7, 2020
Merged

Objentry additions #95

merged 17 commits into from
May 7, 2020

Conversation

Rikux3
Copy link
Contributor

@Rikux3 Rikux3 commented May 7, 2020

Updated some previous unknown fields and the documentation

Copy link
Contributor

@Xeeynamo Xeeynamo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good with an exception IMHO:
On Objentry.cs:59, by specifying an empty description would not lead to an empty label? If you speicfy a [Description] rather than [Description("")] would make it fall back to E_OBJ by default. I did not tested it, so could you confirm if the behaviour is the one you expect to see?

Also I've seen you're having fun with C# Reflection for the Objentry.Clone()! While I do not see any problem in this specific scenario, people on internet can slightly disagree on its usage due to performance or possible bugs. One of them, for example would be if the property does not have a setter, making the field.SetValue fail at runtime and not at compilation time.
But since this is just an opinion not related to this scenario, I would keep as it is in your Pull Request!

@Rikux3
Copy link
Contributor Author

Rikux3 commented May 7, 2020

[Description] and [Description("")] show the same in the UI = an empty string. It seems that the Attribute isn't using the enum name as a fallback. I have removed the empty descriptions nevertheless because they looked ugly.

I didn't like the way i've implemented the Clone function before, because it didn't recognize changes in the structure of the object. I've also thought about implementing the IClonable interface but in the end i've liked the Reflection approach better (although it can have side effects).

@Xeeynamo
Copy link
Contributor

Xeeynamo commented May 7, 2020

Looks good 👍

@Rikux3 Rikux3 closed this May 7, 2020
@Rikux3 Rikux3 reopened this May 7, 2020
@Rikux3 Rikux3 merged commit 3707f4c into master May 7, 2020
@Rikux3 Rikux3 deleted the objentry-improvements branch May 7, 2020 13:39
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

Successfully merging this pull request may close these issues.

2 participants