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

fix: enable hot reload on windows #290

Merged
merged 1 commit into from Oct 17, 2022

Conversation

guilefoylegaurav
Copy link
Contributor

@guilefoylegaurav guilefoylegaurav commented Oct 7, 2022

Description

Concerned with issue/bug #283

Changes

  • fix(dev_event_handler.py): use platform specific python3 alias to spawn processes

Documentation

No documentation update is required

How I've tested my work

  • 26/26 tests passed (Debian 10)
  • 25/26 tests passed (Windows 10) (global_index_request() failed)

Remarks

  • Although this minute code change gets the dev_index_request() test passed on windows, for some reason hot reload fails to work on my system.

    • Windows: Inside dev_event_handler.py, function on_any_event() gets called every time I make a change in the base_routes.py file but I cannot see changes being reflected on the browser.
    • Debian: Inside dev_event_handler.py, function on_any_event() does not get called when I make a change in the base_routes.py file and thus I cannot see changes being reflected on the browser.
    • I just want to make sure if this is a problem that everyone else is experiencing or is it only me?
  • Failure of global_index_request() test persists

@netlify
Copy link

netlify bot commented Oct 7, 2022

Deploy Preview for robyn canceled.

Name Link
🔨 Latest commit 2e4b8b9
🔍 Latest deploy log https://app.netlify.com/sites/robyn/deploys/634aee30dd25af0009e8b5a8

@sansyrox
Copy link
Member

sansyrox commented Oct 7, 2022

Hi @guilefoylegaurav , thanks for your PR.

I don't have an access to a windows machine till the 26th so can't really tell you about the windows issue.

Dev server reflects the right changes on my osx machine but is not killed properly on KeyboardInterrupt. I will check my linux PC to give you updates about the debian state.

@@ -11,7 +12,8 @@ def __init__(self, file_name) -> None:
def start_server_first_time(self) -> None:
if self.processes:
raise Exception("Something wrong with the server")
self.processes.append(subprocess.Popen(["python3", self.file_name], start_new_session=False))
command = "python3" if not sys.platform.startswith("win32") else "python"
Copy link
Member

Choose a reason for hiding this comment

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

instead of this we can create a private computed python property that gives us a python alias depending on the platform. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct. That would be much better.

@sansyrox
Copy link
Member

sansyrox commented Oct 8, 2022

@guilefoylegaurav , I think you pushed the same commit.

@guilefoylegaurav
Copy link
Contributor Author

guilefoylegaurav commented Oct 9, 2022

Noticing that there were some updates to the upstream, I pulled the updates down from upstream to the fork's main and rebased the feature branch with the main and push that back, actually

@sansyrox
Copy link
Member

@guilefoylegaurav , are you working on converting duplicate code to a property?

@guilefoylegaurav
Copy link
Contributor Author

My apologies
I am, just pushed the updates

@sansyrox
Copy link
Member

Just one last thing @guilefoylegaurav , have you run the pre-commit instructions in the README here?

https://github.com/sansyrox/robyn#%EF%B8%8F-to-develop-locally

@guilefoylegaurav
Copy link
Contributor Author

guilefoylegaurav commented Oct 12, 2022

@sansyrox
I did not run it until now
Ran it now and it did not reveal any formatting issues

I was under the impression that the pre-commit runs automatically on git commit

@sansyrox
Copy link
Member

Ah, okay. This PR looks good. But I will do a final review tomorrow morning 😄

Also, the link that you shared is dead in the last comment.

@guilefoylegaurav
Copy link
Contributor Author

guilefoylegaurav commented Oct 14, 2022

Thanks @sansyrox
My bad, I updated the link
I am embarrassed beyond words - I messed the link up while copying it seems. I will take care from now on to ensure that this does not happen again

@@ -7,11 +8,14 @@ class EventHandler(FileSystemEventHandler):
def __init__(self, file_name) -> None:
self.file_name = file_name
self.processes = []
self.alias = "python3" if not sys.platform.startswith("win32") else "python"
Copy link
Member

Choose a reason for hiding this comment

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

One last nit: can you name it as python_alias?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@sansyrox
Copy link
Member

I am embarrassed beyond words - I messed the link up while copying it seems. I will take care from now on to ensure that this does not happen again

Not to worry 😄 Happens with all of us 😄

* fix(dev_event_handler.py): use platform specific python3 alias to spawn processes
Copy link
Member

@sansyrox sansyrox left a comment

Choose a reason for hiding this comment

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

Great work! LGTM 🔥

@sansyrox sansyrox merged commit 0b8d685 into sparckles:main Oct 17, 2022
Shending-Help pushed a commit to Shending-Help/robyn that referenced this pull request Oct 19, 2022
* fix(dev_event_handler.py): use platform specific python3 alias to spawn processes
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

2 participants