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

[feat]: Added port already in use warning. #558

Closed
wants to merge 0 commits into from

Conversation

Mr-Sunglasses
Copy link
Contributor

Added port already in use warning and added to ask the user for the prompt to use the new port.

Description

port-patch.mov

This PR fixes #508, #557

@netlify
Copy link

netlify bot commented Jul 17, 2023

👷 Deploy request for robyn pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 2ffd591

@Mr-Sunglasses
Copy link
Contributor Author

@sansyrox I think the feature became a bug for the tests in Windows 😅 . The GHA is running all the tests for windows on the same machine at the same time I guess that is why that port is already in use warning/error is there 🥲 .

robyn/processpool.py Outdated Show resolved Hide resolved
robyn/__init__.py Outdated Show resolved Hide resolved
@Mr-Sunglasses
Copy link
Contributor Author

@sansyrox Fixed all the requested changes. 🕺🏻

@Mr-Sunglasses
Copy link
Contributor Author

@sansyrox Fixed all the requested changes. 🕺🏻

@sansyrox I think more test are failing now after using websockets 😢

robyn/__init__.py Outdated Show resolved Hide resolved
@codspeed-hq
Copy link

codspeed-hq bot commented Aug 5, 2023

CodSpeed Performance Report

Merging #558 will not alter performance

Comparing Mr-Sunglasses:port-use-patch (6ae6e67) with main (2b39ae5)

Summary

✅ 100 untouched benchmarks

@sansyrox sansyrox force-pushed the main branch 5 times, most recently from 71f36b6 to 416bc97 Compare August 6, 2023 16:00
Comment on lines 29 to 32
except OSError:
return True
except Exception:
return True
Copy link
Member

Choose a reason for hiding this comment

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

Hey @Mr-Sunglasses 👋

One suggestion - let us return errors here. It is a recommended way to
not do a generic catch all for the Exception

@sansyrox
Copy link
Member

Hey @Mr-Sunglasses 👋

Any updates on this PR ?

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.

Assign a new port if the current one is taken
2 participants