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.

Sign up for GitHub

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

Jump to bottom

Readonly structs #1130

Closed
sonnemaf opened this issue Aug 31, 2022 · 7 comments · Fixed by #1135
Closed

Readonly structs #1130

sonnemaf opened this issue Aug 31, 2022 · 7 comments · Fixed by #1135
Labels
enhancement

Comments

@sonnemaf
Copy link

Should/could the structs in this project be made readonly. I think they should. All primitive struct types from Microsoft are readonly.

Readonly structs have a lot of advantages where there are no defensive copies for readonly fields, in parameters, 'readonly locals + returns`.
@Brunni
Copy link
Contributor

Brunni commented Sep 2, 2022

As far as I see, everything compiles with public readonly partial struct as well.

@angularsen
Copy link
Owner

Sounds like a good improvement as long as it compiles for nestandard2.0.
These structs were intended to be immutable already.

Would you be interested in attempting a pull request @sonnemaf ?

@sonnemaf
Copy link
Author

sonnemaf commented Sep 7, 2022

Hi @angularsen

Good news but unfortunately I don't have time to implement it the next few weeks. Is it in a hurry? I will probably have time for it in october.

Brunni added a commit to Brunni/UnitsNet that referenced this issue Sep 10, 2022
@Brunni
Use readonly for structs in QuantityGenerator angularsen#1130
5d3d076
Brunni added a commit to Brunni/UnitsNet that referenced this issue Sep 10, 2022
@Brunni
Use readonly for structs in QuantityGenerator angularsen#1130
714ddb4
@Brunni Brunni mentioned this issue Sep 10, 2022
Merged
@angularsen
Copy link
Owner

angularsen commented Oct 11, 2022 via email

@sonnemaf
Copy link
Author

Hi,

I'm unable to clone the repository. I keep getting this error. Tried it on two computers.

image

I'm giving up. The change is really small. You only have to add the readonly text to the UnitsNet\CodeGen\Generators\UnitsNetGen\QuantityGenerator.cs on line 75, see screenshot below.

image

Can you do it yourself?

Regards,

Fons

@Brunni
Copy link
Contributor

Brunni commented Oct 31, 2022

There is already a PR for this. #1135
Currently the maintainer @angularsen seems to be unavailable.

angularsen pushed a commit that referenced this issue Nov 2, 2022
@Brunni
Use readonly for structs in QuantityGenerator (#1135)
dfd1afc 
Fixes #1130

https://docs.microsoft.com/en-us/dotnet/csharp/write-safe-efficient-code#declare-immutable-structs-as-readonly

Force the compiler to check, that all fields are readonly as well.
@angularsen
Copy link
Owner

@sonnemaf Sorry for the friction. You need to install Git LFS to clone the repo.
https://git-lfs.github.com/

LFS is nice when it works, but GitHub charges for it when monthly bandwidth exceeds a certain amount.
#1052 suggests downloading tooling binaries on-demand with an init script, instead of having it happen during checkout.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Use readonly for structs in QuantityGenerator Brunni/UnitsNet
3 participants
@angularsen @Brunni @sonnemaf