Skip to content

Refactor up/down moves #21

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
Merged
chshersh merged 6 commits into from
Jan 24, 2025
Merged

Refactor up/down moves #21

chshersh merged 6 commits into from
Jan 24, 2025

Conversation

@heathhenley
Copy link
Collaborator

@heathhenley heathhenley commented Jan 17, 2025

Attempt to close #17

@chshersh
Copy link
Owner

chshersh commented Jan 18, 2025

I propose to focus on the other two PRs first and get them merged:

Once they're done, I'll review this one 🙂

@chshersh chshersh added type: refactoring Improving the code quality w/o changing functionality component: DX Linters, CI, developer experience improvements labels Jan 18, 2025
@chshersh
Copy link
Owner

chshersh commented Jan 21, 2025

Since the other PRs are merged, I propose to update this one, and I'll review it

@heathhenley heathhenley force-pushed the dev_hh_ocaml_practice branch from 08789e3 to 5031e30 Compare January 21, 2025 22:44
@heathhenley
Copy link
Collaborator Author

heathhenley commented Jan 21, 2025

Just rebased on the latest main

chshersh
chshersh reviewed Jan 22, 2025
View reviewed changes
Copy link
Owner

@chshersh chshersh left a comment

Choose a reason for hiding this comment

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

Great stuff!

I left a bunch of nitpicks, mostly some stylistic improvements.

After looking at the diff, it may seem like this refactoring doesn't do a lot. But in the future, there'll be lots of zipper-like and scrollable elements, so it makes sense to prepare the ground for this work and start abstracting some of these movements.

chshersh
chshersh reviewed Jan 23, 2025
View reviewed changes
@heathhenley heathhenley force-pushed the dev_hh_ocaml_practice branch from 5fe3a10 to bf78c1b Compare January 23, 2025 22:07
@heathhenley
Copy link
Collaborator Author

heathhenley commented Jan 23, 2025 •
edited
Loading

I just rebased on the latest main and added the suggestion you had above to clean up the offset check logic. I also rewrote to use pipes but not sure if it's actually better here, lmk what you think!

chshersh
chshersh reviewed Jan 23, 2025
View reviewed changes
@heathhenley @chshersh
Update lib/fs/fs.ml
175dca9 
Co-authored-by: Dmitrii Kovanikov <[email protected]>
chshersh
chshersh approved these changes Jan 24, 2025
View reviewed changes
Copy link
Owner

@chshersh chshersh left a comment

Choose a reason for hiding this comment

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

Thanks for the refactoring!

@chshersh chshersh merged commit acddf58 into chshersh:main Jan 24, 2025
3 checks passed
@chshersh chshersh mentioned this pull request Jan 24, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chshersh chshersh chshersh approved these changes

Assignees

@heathhenley heathhenley

Labels

component: DX Linters, CI, developer experience improvements type: refactoring Improving the code quality w/o changing functionality

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Refactor fs.ml: Separate concerns in go_down and go_up functions

2 participants

@heathhenley @chshersh