Skip to content
This repository has been archived by the owner on Mar 28, 2023. It is now read-only.

2.8 ic packet extension no empty strings #144

Closed
wants to merge 44 commits into from
Closed

2.8 ic packet extension no empty strings #144

wants to merge 44 commits into from

Conversation

caleb-mabry
Copy link
Contributor

Potential solution for #107 ?? I'm not sure. There's a whole lot going on in that method.
This is also somewhat of a refactor and may not be what you're looking for.

Adding all of the typing was absolutely killing me. I used the @dataclass decorator to try and slim down some of the difficulty in reading the massive type checking. Then I created separate dataclasses for each pre-existing packet type and had each of those inherit from one another so we can just use the 2.8 packet standard for everything. Using something I found on stack overflow, I'm able to use @enforce_typing decorator on the @dataclass decorator and it enforces the typing.

I wouldn't say that this PR is ready to go yet, but I would like to get some guidance on whether or not this is addressing the issue. It seems like it type-checks for what you're looking for.

Any feedback is appreciated!

@caleb-mabry
Copy link
Contributor Author

Ignore the changes in client manager. I forgot to revert

Copy link
Member

@oldmud0 oldmud0 left a comment

Choose a reason for hiding this comment

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

That type enforcement file made me excited and I almost didn't finish the review

@caleb-mabry
Copy link
Contributor Author

All of the previously addressed concerns should have been handled, feel free to take a look at them and provide additional feedback.

Copy link
Member

@oldmud0 oldmud0 left a comment

Choose a reason for hiding this comment

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

Yeah looks good. Still a lot of refactoring to be done. Just try not to go overboard with generics and typing so regular people can understand it.

@caleb-mabry
Copy link
Contributor Author

I think one of my big hopes by even adding typing is to fix where the code will have a 0 as a string rather than an int. That alone can cause people to get confused IMO

@caleb-mabry
Copy link
Contributor Author

Oh that is such a fat Merge conflict

@caleb-mabry
Copy link
Contributor Author

And after a long challenge of merging. I am pleased with this and it is ready

@caleb-mabry
Copy link
Contributor Author

Did we want me to update more on this?

I feel like I might've made this PR too big and I should've just changed the args to an object and left some of the logic still in the same spot

@oldmud0
Copy link
Member

oldmud0 commented Jan 31, 2021

No sorry I had just been waiting on CI. Looking back at it though, this is a fairly large refactoring PR, and some further testing would be nice.

@caleb-mabry caleb-mabry closed this Feb 1, 2021
@caleb-mabry
Copy link
Contributor Author

I wasn't pleased at all with how large I made this. I'll be making this a smaller PR so that the changes won't have to be so heavily tested

@caleb-mabry caleb-mabry deleted the 2.8-ic-packet-extension-no-empty-strings branch February 1, 2021 02:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants