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

Add new modifier to class methods that hide base class methods #1097

Merged
merged 1 commit into from
Aug 7, 2024

Conversation

adamreeve
Copy link
Contributor

  • I agree that my contribution may be licensed either under MIT or any version of LGPL license.

Resolves #1094

@badcel
Copy link
Member

badcel commented Jul 18, 2024

I'm not sure if the unit tests in this case are of the type I want to have: They are tightly coupled to the implementation. (Especially as I don't like the current Generator.Model approach.)

For generated code I would prefer to have tests that are independent from the generator implementation.

I don't know if this works out but my preferred solution would be to have some new GObject class which has a to_string method. The class would be here: https://github.com/gircore/gir.core/tree/main/src/Native/GirTestLib

There is a special GirTest project to interact with the library.

The warning which is emitted for the missing new / override operator must be converted to an error so the build would fail if it is missing. As the generator is applying the operator the build would not fail. The test would then call the method to verify the expected string is returned.

Additionally the instance can be casted to System.Object to call System.Object.ToString method and verify the original implementation is still accessible.

What do you think?

@adamreeve
Copy link
Contributor Author

Thanks for the feedback, yes that seems like a better approach. I wasn't that happy with how the tests were implemented either but didn't think of changing the test project to treat the warning as an error.

@adamreeve adamreeve force-pushed the new_methods branch 2 times, most recently from ab8ddd1 to ac490fc Compare July 22, 2024 21:08
Copy link
Member

@badcel badcel left a comment

Choose a reason for hiding this comment

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

I checked the code and except the new parameter to force the new modifier it looks fine to me. Let's see what your thoughts on my comment are.

@badcel badcel merged commit c1b996f into gircore:main Aug 7, 2024
3 checks passed
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.

ToString methods hide object.ToString
2 participants