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

Add exclude option #53

Merged
merged 36 commits into from Feb 16, 2022
Merged

Add exclude option #53

merged 36 commits into from Feb 16, 2022

Conversation

mastrzyz
Copy link
Contributor

@mastrzyz mastrzyz commented Jul 19, 2021

We have a problem where we need to get a port for a local but we can't use specific ports due to them being blocked.

In our case, Chromium blocks them -> https://chromium.googlesource.com/chromium/src.git/+/refs/heads/main/net/base/port_util.cc

Here I added logic for a exclude generator in the same way we have the makeRange generator.

For example :

getPort.exclude([1024,1027])

will give me a generator containing all ports from the valid TCP range excluding 1024 and 1027

[1025,1026,1028,1029,...,65535]

@mastrzyz
Copy link
Contributor Author

@sindresorhus let me know if I need to change anything here. Lovely tool you have in this repo!

@sindresorhus
Copy link
Owner

I think it should be an option instead. Having exclude as a separate method makes it not compose well with makeRange(). The exclude option could support any Iterable, so you could use .makeRange to make the exclusion range.

@mastrzyz
Copy link
Contributor Author

aaa , so something like this ( current update )

@sindresorhus
Copy link
Owner

Yes, but it shouldn't even run getAvailablePort for the excluded ports.

@mastrzyz
Copy link
Contributor Author

Ok I think I got it , let me know

@sindresorhus sindresorhus mentioned this pull request Jul 26, 2021
@mastrzyz
Copy link
Contributor Author

@sindresorhus , let me know of anything else needing to be done here, thanks!

@sindresorhus
Copy link
Owner

You haven't actually done what was asked: #53 (comment)

index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
@mastrzyz
Copy link
Contributor Author

You haven't actually done what was asked: #53 (comment)

You haven't actually done what was asked: #53 (comment)

Not sure I follow, isn't that what this statement is for?

image

@sindresorhus
Copy link
Owner

@mastrzyz I'm not entirely sure what you're asking. Please just read the comment I linked to:

Having exclude as a separate method makes it not compose well with makeRange(). The exclude option could support any Iterable, so you could use .makeRange to make the exclusion range.

index.d.ts Outdated Show resolved Hide resolved
@mastrzyz mastrzyz closed this Aug 2, 2021
@mastrzyz
Copy link
Contributor Author

mastrzyz commented Aug 2, 2021

Closing this PR , don't have the cycles currently to continue work on this.

@sindresorhus sindresorhus mentioned this pull request Aug 3, 2021
@mastrzyz
Copy link
Contributor Author

I have cycles again and will start work here.

@mastrzyz mastrzyz reopened this Sep 15, 2021
@mastrzyz
Copy link
Contributor Author

@sindresorhus I have updated all unresolved comments to the best of my knowledge.

readme.md Outdated Show resolved Hide resolved
index.js Show resolved Hide resolved
@sindresorhus sindresorhus changed the title Adding exclusion logic to get-port Add exclude option Jan 26, 2022
index.js Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
test.js Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Owner

I would recommend looking over your own diff to ensure there are no more typos, inconsistencies or things that could be improved: https://github.com/sindresorhus/get-port/pull/53/files

@sindresorhus sindresorhus merged commit d03c07b into sindresorhus:main Feb 16, 2022
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.

None yet

3 participants