Skip to content

Conversation

@dgjustice
Copy link
Contributor

@qduk I have refactored the gist I sent (thank you for pointing out how broken that was! 😬). I finally put together a proper PR with all the test cases I could find. This is a bit more complexity, but taking the "character class" approach allows future modifications to completely customize the sorting logic based on arbitrary groups of characters. Sorts happen first by CharacterClass.weight, and secondly by whatever logic you like, for example, casting to int for the CCInt class.

Copy link
Collaborator

@qduk qduk left a comment

Choose a reason for hiding this comment

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

@dgjustice gotta say, this is pretty awesome stuff. I've just got one other note.

Docs is picking up on your character classes and helper functions. I personally don't think those need to be in there. I am able to get them undocumented by making them private functions. I'll defer to @itdependsnetworks and @jeffkala to see if they agree or not.

Thanks again @dgjustice. Once this is merged it'll allow me to build out my PR.

Co-authored-by: Adam Byczkowski <[email protected]>
@jeffkala
Copy link
Collaborator

jeffkala commented Aug 2, 2021

@dgjustice gotta say, this is pretty awesome stuff. I've just got one other note.

Docs is picking up on your character classes and helper functions. I personally don't think those need to be in there. I am able to get them undocumented by making them private functions. I'll defer to @itdependsnetworks and @jeffkala to see if they agree or not.

Thanks again @dgjustice. Once this is merged it'll allow me to build out my PR.

I can't say I have a huge issue either way on whether those show up in the docs. To me more is always better in docs, but I understand the concern.

Copy link
Collaborator

@jeffkala jeffkala left a comment

Choose a reason for hiding this comment

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

This is an awesome addition. Learned a ton just from reading over the code. Thanks @dgjustice

@dgjustice dgjustice requested a review from qduk August 2, 2021 19:08
@dgjustice
Copy link
Contributor Author

Travis is being flaky again. I think the last commit is clean (passes tests); the build failed on installing dependencies.

Copy link
Collaborator

@qduk qduk left a comment

Choose a reason for hiding this comment

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

@jeffkala You make a good point. Never hurts to have the extra documentation.
@dgjustice Thanks for adding the docstring example.

@qduk qduk merged commit 57972bc into networktocode:develop Aug 6, 2021
@dgjustice dgjustice deleted the dgjustice branch August 6, 2021 22:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants