Skip to content

picoev: refactor - #20558

Merged
spytheman merged 25 commits into
vlang:masterfrom
enghitalo:refactor/picoev
Feb 25, 2024
Merged

spytheman merged 25 commits into
vlang:masterfrom
enghitalo:refactor/picoev

Conversation

@enghitalo

Copy link
Copy Markdown
Contributor

No description provided.

Comment thread vlib/picoev/picoev.v Outdated
Comment thread vlib/picoev/picoev.v Outdated
Comment thread vlib/picoev/picoev.v Outdated
Comment thread vlib/picoev/picoev.v Outdated
Comment thread vlib/picoev/picoev.v
Comment thread vlib/picoev/picoev.v Outdated
Comment thread vlib/picoev/picoev.v Outdated
@spytheman

Copy link
Copy Markdown
Contributor

@enghitalo please avoid refactoring/renaming in the same PR, as functional changes. I know that it is sometimes unavoidable, but here, more than half of the changes are renames.

Intermixing both in the same PR makes it much harder to read and approve, and the cost in time is not linear, especially if there are problems found. You (and I) will have to read those diffs over and over and over again, till everything is good to merge, while if those were 2 separate PRs (one pure refactoring, and another with potentially breaking, but surely functional changes), the pure refactoring one would have been approved and merged pretty much immediately, and the diff for the second one, would have been much smaller and focused.

@spytheman

spytheman commented Jan 16, 2024 •

Copy link
Copy Markdown
Contributor

Think of it as wading through a pile of straw (the renames), to search for a pearl, or a needle potentially hidden in it.

@enghitalo
enghitalo marked this pull request as draft January 17, 2024 02:40
@enghitalo

Copy link
Copy Markdown
Contributor Author

@spytheman Thank you for your review and tips. I will try finish it next week

@enghitalo
enghitalo marked this pull request as ready for review February 25, 2024 14:45
@spytheman
spytheman merged commit 9703029 into vlang:master Feb 25, 2024
raw-bin pushed a commit to raw-bin/v that referenced this pull request Mar 9, 2024
raw-bin pushed a commit to raw-bin/v that referenced this pull request Mar 9, 2024
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.

3 participants