Closed AndrzejKebab closed 11 months ago
A few things: Foremost: Each pull request should address a specific issue. This PR has many commits and touches 3-4 different areas. Also, typically when creating a PR that has this many commits in it you should squash most (or all) of them and rebase --onto so it's easy to review. The exception to this is when you want to show other developers your step by step development process.
a) I don't think it's necessary to cover every single primitive edge case with an Extension method (byte, short, int, float, half, long, double). It's enough to have the most common ones, and people can supplement their own projects with more specific types as necessary, using the provided examples. Otherwise we are going to end up with a library that is bloated with YAGNI code. YAGNI is one reason why there is only one version of PercentageOf. The other reason is that you don't have to cast a float when dividing floats to get an accurate percentage. It's also unclear why the byte methods are comparing against short values.
b) Keeping the Mathf extension methods in a struct signals to other developers that this object has no dependencies, when in fact it does have one dependency (the Unity.Mathematics library, if available). Most devs would expect this to be a static class instead.
c) .gitattributes is unnecessary in this library - consumers will have their own version, if any.
d) The changes to .gitignore also appear to be very project specific. It assumes the consumer has nested folders with these names, which is unusual. Again, this would be project specific, and doesn't need to be changed here. The original file is just here as an example.
e) Many methods have more documentation than code - the NumberExtensions class is a wall of repetitive comments and very little actual code - what benefit are these comments really adding here? All of these methods are self-explanatory. If a comment does not add additional information, then all it is doing is adding visual clutter. https://stackoverflow.blog/2021/12/23/best-practices-for-writing-code-comments/
So, in summary - these are some good changes, however there is a fair amount of YAGNI here and changes which are atypical of the average Unity project. My suggestions:
1) Reduce complexity - NumberExtentions and MathfExtensions should only provide int, float, half and double methods. Remove byte, short and long as these can easily be added by consumers on an as needed basis. 2) Remove excessive documentation on simple extension methods that don't add additional information or clarity. 3) Change MathfExtensions.cs from struct to a static class 4) Roll back changes to .gitignore and remove .gitattributes file
This looks pretty good. Just one more thing. It looks like when reverting the .gitignore
file it actually added a double backslash to each entry. You could just checkout the original file fromorigin/master
a) I use mostly byte, short and half so I added them b) I didn't hear anywhere that struct shouldn't have dependencies, i did it as struct because I wanted to make this as extensions for Mathf but it don't work like for what is already, so i changed it and leaved it as struct d) It was my for this repo because I used unity to do this for autocompletion etc. and to not push that all unity garbage i edited .gitignore e) This was for people like me, AtLeast what? AtMost what? I didn't know what this is for until chat gpt told me
should be good i hope
Looks good. I think I understand your logic behind a-e comments now. On future PRs, if you could add some detail like that when opening the PR, it will help me evaluate the changes a bit better/faster I think. Cheers, and Happy New Year!
I hope you don't mind that I use your repository as a field for learning and Happy New Year!
I wonder why
PercentageOf
takesthis Int
and notthis Float