Skip to content

Modernization#38

Merged
rzr merged 18 commits into
abandonware:masterfrom
akx:modernize
Feb 20, 2020
Merged

Modernization#38
rzr merged 18 commits into
abandonware:masterfrom
akx:modernize

Conversation

@akx
Copy link
Copy Markdown

@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
Copy Markdown

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
Copy Markdown
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
Copy Markdown

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/

npm test
- name: Lint
run: |
if [ ${{ matrix.node }} = 6 ]; then
Copy link
Copy Markdown

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
Copy Markdown
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+.

Comment thread test/test-service.js
@akx
Copy link
Copy Markdown
Author

akx commented Feb 17, 2020

@rzr Rebased.

@rzr
Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown

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
Copy Markdown

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
Copy Markdown

rzr commented Feb 20, 2020

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

siegfried-rz pushed a commit to rZeroSys/noble that referenced this pull request Aug 1, 2025
previously, it would check for the peripheral role (act as BLE devices) instead of central role (interact w/ BLE devices)
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