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

refactor: modernize code using es6 classes #206

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

danielpza
Copy link
Contributor

@danielpza danielpza commented Aug 22, 2024

Modernize code by replacing function constructors with modern ES6 classes

See https://github.com/tj/node-migrate/pull/206/files?diff=unified&w=1 for a better diff of the PR

@danielpza danielpza marked this pull request as ready for review August 22, 2024 02:11
@wesleytodd
Copy link
Collaborator

I am not a big fan of "modernize" PRs for the sake of "modernization". Do you have more concrete reasons to prefer this?

I am not against it, and I would never write new code with the old style, it is just "change for changes sake" is rarely a good thing.

@danielpza
Copy link
Contributor Author

I am not a big fan of "modernize" PRs for the sake of "modernization". Do you have more concrete reasons to prefer this?

No concrete reason, it would make it easier to maintain if we use a more familiar code (in this case classes instead of the obscure function prototypes).

@wesleytodd
Copy link
Collaborator

I generally agree with that sentiment. I started a whole rewrite I intended to release as the next major in fact, just never had time to circle back on it. So I am just not sure if this kind of change in such a slow paced project is worth it. I dont want to decline a good and well meaning contribution though.

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.

2 participants