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

Make linting rules more strict #293

Merged
merged 1 commit into from
Jun 8, 2023
Merged

Conversation

cjbarth
Copy link
Contributor

@cjbarth cjbarth commented Jun 1, 2023

This PR aims to improve code quality and consistency by tightening up some linting rules slightly. These rules require Node >=4 because of the rule to prefer let over var.

@cjbarth
Copy link
Contributor Author

cjbarth commented Jun 1, 2023

@yaronn , this is a draft right now because we don't pass all the rules. I'd like your feedback on two things:

  1. Do these rules seem reasonable to you?
  2. Do you think that setting the minimum require engine version in package.json to 4 would be a semver-major breaking change, or can we just do that right now. For referenced, our minimum required Node version is >=0.4.0 which was released in 2011. It seems to me that perhaps when that was entered, the idea was to make the minimum required version >=4.0.0 instead of >=0.4.0, the latter being a typo.

@shunkica
Copy link
Contributor

shunkica commented Jun 6, 2023

Maybe you have some other information but as far as I can tell @yaronn has not been active anywhere for over 5 years.

@cjbarth
Copy link
Contributor Author

cjbarth commented Jun 6, 2023

@shunkica , you're right. I meant to do @LoneRifle :)

@cjbarth cjbarth requested a review from LoneRifle June 6, 2023 15:22
@LoneRifle
Copy link
Collaborator

LoneRifle commented Jun 7, 2023

Rules make sense to me. I think targeting 0.4.0 was what yaronn deliberately intended, see eg #77. That said, even with Travis, we've been testing against newer versions of Node, so perhaps it may make sense to tweak engine to line up with whatever we are supporting on our CI now, especially since the package's dependencies need at least Node 4, and with Mocha, even more recent versions of Node.

Copy link
Collaborator

@LoneRifle LoneRifle left a comment

Choose a reason for hiding this comment

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

lgtm

@cjbarth cjbarth merged commit dedb9c3 into node-saml:master Jun 8, 2023
@cjbarth cjbarth deleted the strict-linting branch June 8, 2023 00:08
@cjbarth cjbarth added the chore label Jun 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants