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

update to work with Sails 0.12.x #14

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

Conversation

markwbrown
Copy link

config is a reserved name in 0.12.x -- updated to use bunyanConfigObject. Also added .DS_Store to .gitignore, updated README.md to signify compatibility with 0.12.x, updated Changelog.md to show changes and why. 80 characters is silly and per the comments of this package's logLevels object, there is "no place for silly around here". It has been over 10 years since Linus Torvalds proposed 132 characters as a reasonable limithttps://lkml.org/lkml/2009/12/17/229, citing "80 characters is causing too many idiotic changes."

…t config. update changelog and readme, also, update lint rules to more sensible number of characters per line because nobody uses a vt100 and bump version in package.json
@leedm777
Copy link
Contributor

leedm777 commented Aug 6, 2020

@markwbrown,

There are a number of problems with the PR, but the most fundamental is that it doesn't actually do anything. Can you point me to the error message and line of code in Sails that's complaining about the use of the name config?

Replying to your comment on your other PR:

I'm glad you found it in poor taste enough to reply as it is evident you were not actively maintaining this package. There have been no updates in 3 1/2 years and I don't think you probably would have looked at this unless I offended your sensibilities a bit ;-).

I'm not sure why you're glad. I make an effort to reply on all my side projects, even those I'm not actively using or maintaining any more.

I had actually intended for commit 7ce6579 (now gone -- I cleaned everything up and forced a push to my fork) as the merge point, but made some other changes after I submitted the pull request in order to post the package to our organization's npm, but whatever.

You can create a branch in your repo to submit the PR from, keeping the PR changes separate from whatever you need for publishing your fork.

Sure, it seems like a completely arbitrary change, but in Sails 0.12 onward config is reserved. It's been that way since 2016.

Honestly, it still looks completely arbitrary. Reserved in what way?

I have no interest in a copyright claim or being on the license. As I continue to upgrade the application, we'll see if there are any more changes to your package that might be worthy of being listed as a contributor. I have restored your copyright claim and submitted a new pull request.

Thank you. And thank you for updating the changelog.

The only difference between the pull request and yours that should definitely be changed in any merge is removing the scope in the package name (needed for our org's npm registry).

See above comment about creating a branch in your repo.

80 character line lengths are dumb unless you're on a vt100. That is a point I feel strongly about.

Agree to disagree.

@markwbrown
Copy link
Author

Error defining hook: config is a reserved property and cannot be used as a custom hook method.

@markwbrown
Copy link
Author

Most of the time, when people have a repo that has not been updated in years, they don't respond to pull requests at all. Just my experience. I'm glad you responded. When I get some free time, I'll submit a PR from a clean branch. You're both right and wrong that the PR doesn't do anything. It changes a variable name that sails otherwise complains about in 0.12.14, possibly earlier -- I haven't checked.

@markwbrown
Copy link
Author

Agree to disagree on the 80 characters, and I'm right ;-)

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