-
-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
General cleanup - Docblock fixes, adding type hinting, added tests, added asset model form validator #15029
Conversation
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
PR Summary
|
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels like we're moving into really modern code, and I'm hoping it helps us catch things at interpret-time or develop-time way, way before we make it into production. You sneaked a few changes I wouldn't have minded seeing separately into this one, but they're all good fixes so I'm not going to fight too hard. This looks like it was a tremendous amount of work. Thank you for dedicating the time to do it - I'm really hoping it pays off in the future - as we stave off bugs we didn't need to write, or catch them way, way before they even make it into CI/CD testing.
Signed-off-by: snipe <snipe@snipe.net>
Signed-off-by: snipe <snipe@snipe.net>
This touches a buttload of files, but as you can see, the vast majority are just small changes like changing
Auth::user()->id
toauth()->id()
, etc to modernize the codebase a little. I also pulled out references touse
statements that the code wasn't actually using, and added type-hinting to a lot of the controller methods (that last bit is the majority of the files changed).I also added a bunch of tests. We do seem to have a few different naming conventions going on in there and I wasn't sure which one we wanted to settle on.
Not sure if we want to go with:
Or
I don't have strong feelings, but they should be consistent.
Some of these tests only cover the basics - can the index be loaded, etc - but they're at least something more than what we had.
In writing those tests, I did find a few bugs which I also fixed. Those will be the only changes that stand out here, as everything else is pretty straightforward.
For some reason, these changes have made our codacy code quality plummet, even though most of these things are just tests and type-hinting. Seems the type-hinting is somehow increasing our cyclomatic and Npath complexity somehow?