Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Couple of misc refactors #959

Merged
merged 4 commits into from
Jan 21, 2025
Merged

Couple of misc refactors #959

merged 4 commits into from
Jan 21, 2025

Conversation

bjorn3
Copy link
Collaborator

@bjorn3 bjorn3 commented Jan 20, 2025 •
edited
Loading

  • In the sudoers parser use a single fixed stream type. There is no need to be generic over the type of character stream.
  • Remove defaults for the UnixUser trait methods. They aren't used outside of a test anyway.

@bjorn3 bjorn3 force-pushed the misc_refactors branch 3 times, most recently from 8253b0e to b411379 Compare January 20, 2025 13:31
@bjorn3
Copy link
Collaborator Author

bjorn3 commented Jan 20, 2025

The last commit turned into a bit of a rabbit hole, so I've split it out into #961 together with a whole bunch of other refactorings to that code.

@bjorn3 bjorn3 requested a review from squell January 20, 2025 13:32
@squell
Copy link
Member

squell commented Jan 21, 2025

This is interesting. Since I needed positional information in the test case to solve #61, see commit b4154b4. I think this refactor will also remove the need for that one.

I'll see if rebasing the current work on top of this one works but have some similar refactoring to do in the meantime.

@squell squell self-assigned this Jan 21, 2025
bjorn3 added 4 commits January 21, 2025 14:51
@bjorn3
Specialize PeekableWithPos to std::str::Chars
60516b8 
And remove the test-only CharStream impl for Peekable<Iter>
@bjorn3
Remove CharStream trait and rename PeekableWithPos to CharStream
072215b 
There was only a single impl of CharStream remaining.
@bjorn3
Remove defaults for UnixUser
faf6fee 
They were only used in a test which tests nothing other than what the
defaults are.
@bjorn3
Get rid of an unnecessary clone in find_item
9d7cd7c
squell
squell approved these changes Jan 21, 2025
View reviewed changes
Copy link
Member

@squell squell left a comment

Choose a reason for hiding this comment

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

Neatly solves a problem I had to fix otherwise.

The original idea for impl CharStream was that this function would be fed raw character data from a file (i.e. without buffering it).

@squell squell enabled auto-merge January 21, 2025 13:53
@squell squell merged commit 4b644d8 into trifectatechfoundation:main Jan 21, 2025
14 checks passed
@bjorn3 bjorn3 deleted the misc_refactors branch January 21, 2025 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@squell squell squell approved these changes

Assignees

@squell squell

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@bjorn3 @squell