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

Resolved Issue #489 : Made FunctionInfo immutable #500

Closed
wants to merge 2 commits into from
Closed

Resolved Issue #489 : Made FunctionInfo immutable #500

wants to merge 2 commits into from

Conversation

lightlessdays
Copy link

Changes made:
[x] Removed the setters from properties like Parameters, StorageClass, and IsDefined.
[x] Initialized the CliImportMember property using init instead of in the constructor. This makes it immutable after construction.
[x] Remove TODO[#489] from the code.

Signed-off-by: @lightlessdays

Copy link
Author

@lightlessdays lightlessdays left a comment

Choose a reason for hiding this comment

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

Haven't changed many lines, yet Git shows that I have changed 80 lines. This is due to the reason that I deleted the TODO comment, which made all the code shift one line up. My changes passed all the tests mentioned in the Contributing Guidelines.

@ForNeVeR
Copy link
Owner

ForNeVeR commented Dec 8, 2023

Actually, the change you did to cause 80 changed lines is to add an old-school namespace. Before it was

namespace Cesium.CodeGen.Contexts.Meta;

internal record FunctionInfo(

And now it is

namespace Cesium.CodeGen.Contexts.Meta
{
    internal record FunctionInfo(

I am not too picky on the code style and will review the change in detail anyway, but I would say that it's better to turn the namespace declaration back to how it was written. But I can roll it back myself when I'll merge the change, so it's not a big deal anyway.

@ForNeVeR
Copy link
Owner

ForNeVeR commented Dec 8, 2023

The unit tests are broken, though (together with build, huh). Could you please take a look?

Of course, I can help if you need.

@lightlessdays
Copy link
Author

Hello. I have made the following changes:

  • Removed the setters from properties like Parameters, StorageClass, and IsDefined.
  • Initialized the CliImportMember property using init instead of in the constructor. This makes it immutable after construction.

Is any other part of the code dependent on this that might break due to these changes? I believe I found one file. Can I make the necessary changes in that file as well?

@ForNeVeR
Copy link
Owner

ForNeVeR commented Dec 8, 2023

Of course, feel free to update the code to make it compile after the refactoring :)

Initialized the CliImportMember property using init instead of in the constructor. This makes it immutable after construction.

I wonder why you moved other properties to the constructor, but left this one in the class block. Is there a particular reason for that I'm missing?

@lightlessdays lightlessdays closed this by deleting the head repository Dec 9, 2023
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