refactor: Reduce amount of warnings from 180 to 17 #183
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Added doc comments to all public fields (phew!) - Added nonempty constructors to entities where a field could be null after creation (like strings). > Some of these constuctors throw an error if app attempted to instantiate with empty values. > Entities still have 0-argument constructors to work with DbSet. - Some entities now have simplified constructors. > For example, the new Purchase constructor sets 8 fields using 3 parameters. > The new Ticket constructor sets 5 fields with 1 parameter. - Fixed tests as a result of breaking them. > Enable `nullable` in test project > Most should be more robust by avoiding magic strings > Test object getters, like a `testUser` getter, reduces code duplication > Redundant fields in test objects have been removed while ensuring the tests still pass. - Removed `LoginAttempt` entity as stated in a TODO comment - Removed pragma warning ignores - Removed unused usings
Codecov Report
@@ Coverage Diff @@
## develop #183 +/- ##
===========================================
+ Coverage 11.55% 12.39% +0.83%
===========================================
Files 142 141 -1
Lines 6680 6865 +185
Branches 455 478 +23
===========================================
+ Hits 772 851 +79
- Misses 5891 5983 +92
- Partials 17 31 +14
|
Created new simplified constructor for UserResponse Remove commented out code Remove an unused using
|
SonarCloud Quality Gate failed. |
|
Really cool that you took the combat on this Omid. Much appreciated. 🙌 I will provide my thoughts in a few days. 🙂 |
Merged
TTA777
added a commit
that referenced
this pull request
Mar 10, 2025
This PR cherry-picks the xml comments created in #183, reduces warnings by marking properties as required, instead of using constructors. Also removes unused using statements, and other low hanging warnings. Note, there is still a fair bit of warnings since the tests haven't been refactored in this PR. I think the best way to address the warnings from there, would be to migrate them to use the builders, though that seems out of scope of this PR. --------- Co-authored-by: Omid Marfavi <21163286+marfavi@users.noreply.github.com>
|
Closed, since #310 was merged in favour of this |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Added doc comments to all public fields (phew!)
Added nonempty constructors to entities where a field could be null after creation (like strings).
Some entities now have simplified constructors.
Fixed tests as a result of breaking them.
nullablein test projecttestUsergetter, reduces code duplicationRemoved
LoginAttemptentity as stated in a TODO commentRemoved pragma warning ignores
Removed unused usings