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

Modernization #38

Merged
merged 18 commits into from
Feb 20, 2020
Merged

Modernization #38

merged 18 commits into from
Feb 20, 2020

Conversation

akx
Copy link

@akx akx commented Jan 25, 2020

  • Replace JSHint with Eslint with semistandard style
    • Fix its kvetches
  • Run some codemods (all code should still be compatible with Node 6 according to node.green)

@rzr
Copy link

rzr commented Jan 25, 2020

Thanks for this patch serie I would prefer for next time to have them submitted in smaller batch (1 per PR would be nice too) then we can track where it break for linux...

I didnt check yet but is it easy to fix CI errors ?

@akx
Copy link
Author

akx commented Jan 25, 2020

I didnt check yet but is it easy to fix CI errors ?

Yeah, they are. Lint warnings, I swear I fixed these but...

@rzr
Copy link

rzr commented Jan 25, 2020

ok good luck
I will release a new version then.

Would you be interested to co-maintain it ?

See procedure at:
https://abandonware.github.io/

@@ -46,3 +46,10 @@ jobs:
fi
npm install
npm test
- name: Lint
run: |
if [ ${{ matrix.node }} = 6 ]; then
Copy link

Choose a reason for hiding this comment

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

I guess lower versions are not working maybe use "-le" instead of "="

Copy link
Author

Choose a reason for hiding this comment

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

The engines stanza in package.json already says the package only supports Node 6+.

@akx
Copy link
Author

akx commented Feb 17, 2020

@rzr Rebased.

@rzr
Copy link

rzr commented Feb 17, 2020

I think it's desirable but I let it open for other @abandonware/reviewers

Ping me in a couple of weeks if it isn't merged

May I suggest to update commit message to add references to origin of this patch :

#38

and upstream too

https://www.slideshare.net/rzrfreefr/updownstreamflows20190411rzr

Thanks again,

@akx
Copy link
Author

akx commented Feb 17, 2020

May I suggest to update commit message to add references to origin of this patch :

#38

I'm not sure I follow. If this is merged with a regular merge commit (as is usually desirable with larger patch sets like this), the merge commit will have a reference to the PR.

@rzr
Copy link

rzr commented Feb 17, 2020

It's not mandatory just a suggestion

well if rebased if will be gone,
and IMHO, adding a depencency to github is not future proof.

@akx
Copy link
Author

akx commented Feb 17, 2020

well if rebased if will be gone,

People generally don't rebase master though :)

and IMHO, adding a depencency to github is not future proof.

An addition to the commit messages referring to Github would also be a dependency on Github...

@rzr
Copy link

rzr commented Feb 19, 2020

Please consider about forwarding patches to upstream
Also add cross references in commit messages and I merge this once done and release a new version.

Then in future I plan to rebase on upstream....

@akx
Copy link
Author

akx commented Feb 19, 2020

Please consider about forwarding patches to upstream

Last activity in any upstream Noble packages has been in 2018. Beyond Github, I have sent the upstream folks several mails both via regular mail and their Open Collective (they're apparently getting sponsor money monthly...) to no avail. I'll certainly begin upstreaming patches if they show any sign of life, but at this point I'd rather not waste more time on that.

@rzr rzr merged commit b4e2b90 into abandonware:master Feb 20, 2020
@rzr
Copy link

rzr commented Feb 20, 2020

Yes I know that's the main reason I created this org
may this be reported to @opencollective to see how can this be handled better
anyway I'll keep updated PR of relevent changes:

Relate-to: #2

@rzr
Copy link

rzr commented Feb 20, 2020

Change set released in:
https://www.npmjs.com/package/@abandonware/noble/v/1.9.2-6

Close:
#44

@rzr
Copy link

rzr commented Feb 20, 2020

I just attempted to upstream one of your untracked change in this PR:
noble#926

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