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.

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

repl: refactor for readability #54824

Closed
wants to merge 1 commit into from

Conversation

avivkeller
Copy link
Member

@avivkeller avivkeller commented Sep 6, 2024

This PR updates the repl class to leverage modern EcmaScript features, including the use of classes.

I refactored the code to make it more readable and accessible for new contributors (I found it quite difficult to understand before the refactor).

Oddly enough, even though the core logic hasn't changed, one test is now failing. I'm submitting this as a draft to invite feedback and help troubleshoot the issue. Currently, the test has been modified to pass.


I'd love to get some feedback on this change.

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. repl Issues and PRs related to the REPL subsystem. labels Sep 6, 2024
@avivkeller avivkeller added wip Issues and PRs that are still a work in progress. and removed wip Issues and PRs that are still a work in progress. labels Sep 6, 2024
@avivkeller
Copy link
Member Author

CC @nodejs/repl

@aduh95 aduh95 added the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 7, 2024
lib/repl.js Outdated Show resolved Hide resolved
@aduh95
Copy link
Contributor

aduh95 commented Sep 7, 2024

I refactored the code to make it more readable and accessible for new contributors (I found it quite difficult to understand before the refactor).

This is quite subjective, and may or may not be true for others – as a point of data, I don't find the PR very easy to review, so I'm not sure the code is much more readable or easy to understand. Given the size of the PR, I don't think it's worth it, it's very easy to miss something.

@avivkeller
Copy link
Member Author

I don't find the PR very easy to review, so I'm not sure the code is much more readable or easy to understand.

Is there any specific reason for that? I'm trying to figure out how to best improve the readability.

If this PR is too big, I can break it down into smaller parts.

@avivkeller avivkeller marked this pull request as ready for review September 7, 2024 13:50
@avivkeller avivkeller force-pushed the repl-refactor branch 5 times, most recently from d810d58 to 03817d4 Compare September 7, 2024 14:15
@avivkeller avivkeller requested a review from aduh95 September 7, 2024 14:19
Copy link

codecov bot commented Sep 7, 2024

Codecov Report

Attention: Patch coverage is 94.35396% with 52 lines in your changes missing coverage. Please review.

Project coverage is 87.62%. Comparing base (5a3c605) to head (03817d4).
Report is 41 commits behind head on main.

Files with missing lines Patch % Lines
lib/repl.js 94.35% 49 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #54824      +/-   ##
==========================================
+ Coverage   87.61%   87.62%   +0.01%     
==========================================
  Files         650      651       +1     
  Lines      182945   183311     +366     
  Branches    35394    35437      +43     
==========================================
+ Hits       160282   160634     +352     
- Misses      15918    15940      +22     
+ Partials     6745     6737       -8     
Files with missing lines Coverage Δ
lib/repl.js 93.80% <94.35%> (-0.32%) ⬇️

... and 36 files with indirect coverage changes

@avivkeller avivkeller added wip Issues and PRs that are still a work in progress. and removed semver-major PRs that contain breaking changes and should be released in the next major version. labels Sep 7, 2024
@avivkeller avivkeller marked this pull request as draft September 7, 2024 17:15
@avivkeller
Copy link
Member Author

Drafting to resolve the test conflicts, allowing this to be a non-semver-major change.

@avivkeller
Copy link
Member Author

I can't seem to narrow down what caused this to happen. I'm going to follow your advise and open a smaller PR.

@avivkeller avivkeller closed this Sep 7, 2024
@avivkeller avivkeller reopened this Sep 7, 2024
@avivkeller avivkeller closed this Sep 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run. repl Issues and PRs related to the REPL subsystem. wip Issues and PRs that are still a work in progress.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants