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

Some dep and other updates #2

Closed
wants to merge 1 commit into from
Closed

Some dep and other updates #2

wants to merge 1 commit into from

Conversation

Martii
Copy link

@Martii Martii commented Jul 10, 2018

  • Don't let contributors add ./node_modules e.g. use ./.gitignore.
  • Fix LICENSE so GH can parse it correctly. SPDX licensing is at their homepage.
  • All deps updated to latest and passing npm test and npm run lint.
  • Add some badges... useful for quick examination/evaluation.
  • Simple line of text for npmjs.com since they don't currently show description in package.json
  • Renamed description to nearly match repository
  • Tell node to use strict mode for this module
  • Version bump to release status... all your other packages are release status instead of pre-release

Closes #1

* Don't let contributors add `./node_modules` e.g. use `./.gitignore`.
* Fix LICENSE so GH can parse it correctly. SPDX licensing is at their homepage.
* All deps updated to latest and passing `npm test` and `npm run lint`.
* Add some badges... useful for quick examination/evaluation.
* Simple line of text for npmjs.com since they don't currently show `description` in package.json
* Renamed `description` to nearly match repository
* Tell *node* to use strict mode for this module
* Version bump to release status... all your other packages are release status instead of pre-release
@Martii
Copy link
Author

Martii commented Jul 10, 2018

Tested on node@10.6.0 with npm@6.1.0 ... PASS OUJS tests. Looks like CI passed too.

Thanks for the consideration/evaluation... and publishing to npmjs.com when ready.


P.S. Going to take some time getting used to standard package requirements. Heh.

@kemitchell
Copy link
Member

@Martii: Very helpful, thanks. I see that the test suites passes with upgraded dependencies. That's really the important thing here, I think.

I'm going to go ahead and push a release with dependency updates. I'm not going to merge your commit, but I'm going to go add you as a collaborator.

Please continue to send pull requests for significant changes, so we can look them over together. The smaller the diff in each PR, the better.

On the style side, I'd prefer to keep this package as low-effort as we can. The fewer things we have to potentially update down the line, the less work we make for ourselves. It's a very simple package. There shouldn't be too much to think about!

@kemitchell kemitchell closed this Jul 10, 2018
@kemitchell
Copy link
Member

1.0.0 is published.

@Martii Martii deleted the someDepUpdates branch July 10, 2018 21:44
@Martii
Copy link
Author

Martii commented Jul 10, 2018

@kemitchell

I'm not going to merge your commit, but I'm going to go add you as a collaborator.

This is going to be detailed since this package has so many sub-dependencies.

Please continue to send pull requests for significant changes

All of these were... so I will need to split them up.

The smaller the diff in each PR, the better.

Will split them up in the future.

On the style side, I'd prefer to keep this package as low-effort as we can.

And on the maintainer side the lack of badges included in this PR will make maintenance more of a chore (e.g. not "low-effort as we can") with a click route of:

  1. https://github.com/jslicense/spdx-is-osi.js (Initial entry point)
  2. https://github.com/jslicense/spdx-is-osi.js/commits/master (peruse latest CI checkmark and using up bandwidth of lots of listed commits)
  3. https://travis-ci.org/jslicense/spdx-is-osi.js/builds/402297130?utm_source=github_status&utm_medium=notification (if there is a failure)
  4. https://www.npmjs.com/packages/spdx-is-osi (Check to see if it is published on npmjs.com from you)

Instead of:

  1. https://github.com/jslicense/spdx-is-osi.js (fini)

Very simple. :)

The fewer things we have to potentially update down the line, the less work we make for ourselves.

Agreed and examined by your coding habits I designed this PR to fulfill that request. I have limited time to manage third-party dependencies too.

It's a very simple package

Agreed.

There shouldn't be too much to think about!

Unfortunately there is...

  1. Since you skipped the dependency update at
    "replace-require-self": "^1.0.1",
    there may be a possible vulnerability when other projects require a specific older version that may have a vulnerability (such as ReDoS ... and the old dependencies before this definitely had one). Now as maintenance chore I need to go check if there is however as I just mentioned if someone points their project to a specific version of that and it has a vulnerability you get the vulnerability in your package. If you specify an exact starting and another project specifies older your project becomes "sane" whereas their project becomes "insane"... but your code will no longer have flat dependencies because your package is "safer". So really in short you are encouraging vulnerabilities in your package.
  2. Choosing not to merge the .gitignore is not wise and is needed as a necessity. Otherwise it's another maintenance chore cherry picking which files in a commit to push. The first thing I noticed when I was testing this module and this module only is that you didn't have this. So I either could use .gitignore for the rest of the projects life or hand pick what was changed via git gui (and I'm not going to mention how extremely detailed it is with git standalone from the CLI).
  3. I am glad you got rid of package-lock.json but now that also has to be added to .gitignore as npm will always regenerate it and is necessary for npm auditing. e.g. you block it permanently and you lose vulnerability assessment.
  4. The license is purely cosmetic but also helps when someone new comes along and doesn't understand licensing. e.g. let GH explain it to them. So now that chore is on this project without that portion.
  5. The description field in the package.json doesn't make any sense... however the repository description does... syncing these prevents yet another maintenance chore of having to explain what it means. Luckily I'm smart enough (hopefully) to figure it out however newbies sometimes aren't. Your package on npmjs.com of this project makes it look like I'd never want to use it since it only has a code fence and explains nothing. Depending on someones involvement they may just skip right past your package since it has no base description. e.g. we all have limited time and without this single line of explanation I would usually move on. I observed your commit of simplifying the README.md and acted as if I was you... e.g. K.I.S.S. principle.

Finally since I had to make this short novel to explain this... that's yet another maintenance chore... which is fine as you want to be involved in each step.

However... if these basic, simple, tasks aren't merged (I'll make new PRs individually) I will have to respectfully decline the invitation to be a collaborator as it's way too much work that you have introduced by skipping these. I hope some of these overtly detailed explanations help you understand why I did this and summarized in the PR commit summary as to why. e.g. they are equal. :)

@kemitchell
Copy link
Member

kemitchell commented Jul 10, 2018

@Martii, I'm glad you find the package useful, and I appreciate the time you took to feed back here.

However, it seems unlikely we'll work together. I don't want to be in a position where publish privilege gives me a trump card over you in design or style decisions, and I'm sure you'd prefer to avoid that kind of situation, as well.

Feel free to fork the package, or to write your own, where you can fully express yourself. I won't be offended.

@Martii
Copy link
Author

Martii commented Jul 10, 2018

Feel free to fork the package

MIT requires that it can be forked. Also there needs to be a minimum uniqueness to the code which since it's so simple it doesn't meet Copyright requirements at this point (but that's another bag of legalese that I usually avoid explaining).

where you can fully express yourself. I won't be offended.

Unfortunately I'm feeling that you are offended... which is unfortunate. You should take the time to read the prior comment fully rather than respond immediately. I feel that you have misunderstood it completely.

However, it seems unlikely we'll work together.

Sure we will... but that's your choice/decision. I literally don't have time to maintain a lot of forks for authors that aren't available. I thought that I would spend some quality time explaining this to you. I don't do this for everyone.

I don't want to be in a position where publish privilege give me a trump card over you in design or style decisions, and I'm sure you'd prefer to avoid that kind of situation, as well.

That's not the case... it's your project. We currently utilize another package for SPDX and it's just as easy not to utilize this package... however at this time is is pertinent to use it. I just thought I'd chime in to give you a hand and if you reject something you reject it (que sera sera) ... just means that I won't be a collaborator/member at this stage. I am perfectly fine with submitting separate PR's for your understanding and convenience (and if you reject it it's on you). Are you perfectly fine with accepting working in a team environment? That's the real rhetorical question here.

One note... I'm a literal person and I don't beat around the bush and I always mean what I say with no malice or animosity.

@jslicense jslicense locked as off-topic and limited conversation to collaborators Jul 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants