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

Reworking view #878

Merged
merged 10 commits into from
Dec 18, 2024
Merged

Reworking view #878

merged 10 commits into from
Dec 18, 2024

Conversation

skiadas
Copy link
Contributor

@skiadas skiadas commented Dec 17, 2024

No description provided.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting that this didn't appear to be a problem when I was testing locally. I could have sworn I had tested with more than one entry.
Would this be better done by simply adding the newline in the toFileLine function instead?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I like that better actually. I'll make that change.

@oscarlevin
Copy link
Member

I removed a hardcoded port and now the tests are passing. I think though that it would be helpful to add a check to see if the current server is actually still up and working. I ran into some issues where some entries in the running_servers file did not get removed but the server was killed (say a power outage). I'll poke around a bit more before merging.

@skiadas
Copy link
Contributor Author

skiadas commented Dec 18, 2024

Hm in theory it's supposed to go through and check them with the isActiveServer method. But maybe the timing of when that happens isn't right? At a quick glance maybe get_running_servers needs to filter the entries based on isActiveServer. I think right now the cleanup attempt only happens in add_server_entry which is probably too late.

Possibly the least code-changing solution would be to add a conditional check in the call to "next" in active_server_for_path_hash, to limit it to those info who also respond True to info.isActiveServer().

@oscarlevin
Copy link
Member

Thanks for that hint, it helped me track down what was happening. I think its working now, but I want to make sure that codespaces work as expected before merging and releasing a new version.

@oscarlevin oscarlevin merged commit 4ac3142 into PreTeXtBook:main Dec 18, 2024
8 checks passed
@oscarlevin
Copy link
Member

Thanks for this. Very nice improvement. This doesn't have any effect on the codespaces. It would probably be worth implementing something similar there, but that's a task for later.

binding: str

@staticmethod
def fromFileLine(line: str) -> RunningServerInfo:
Copy link
Member

Choose a reason for hiding this comment

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

Note that methods in Python should be snake_case: https://peps.python.org/pep-0008/#method-names-and-instance-variables

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's funny that the logging module doesn't follow that convention. I've definitely been bad at following the convention, as my mind really doesn't like it at all.
Would it perhaps be possible to add this as something reported by the styling tools? https://github.com/PyCQA/pep8-naming

Copy link
Member

Choose a reason for hiding this comment

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

I think Java mixes snake_case and camelCase, if I recall? I know it's a convention elsewhere, but I try to stick with pep8 for Python, even if Python itself violates it in some core libraries...

And yeah, really this should be caught by our linter.

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.

3 participants