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 docs to paths.py #1762

Merged
merged 17 commits into from
Jun 1, 2023
Merged

Conversation

gran4
Copy link
Contributor

@gran4 gran4 commented May 8, 2023

No description provided.

@gran4
Copy link
Contributor Author

gran4 commented May 8, 2023

get_vertex_neighbours seems inefficient.
Should I add slots?

@gran4
Copy link
Contributor Author

gran4 commented May 8, 2023

Should I add slots?

Copy link
Member

@eruvanos eruvanos left a comment

Choose a reason for hiding this comment

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

Looks good to me, I am unsure about the usage of google docs style, but I also filed a request in discord.
I would be fine with the change

Copy link
Member

@pushfoo pushfoo left a comment

Choose a reason for hiding this comment

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

tl;dr

  1. Some of these changes are good
  2. However, there are typos in the PR
  3. We should wait on using Google style docstrings since it can break rendering

On Google docstrings

I've experimented with enabling Google docstrings locally & asked about next steps. I agree with einarf's recommendation that we should do the following:

  1. Stick to our current docstring format for now
  2. Use a conversion script to update docstrings in a dedicated PR when we're ready

get_vertex_neighbors

get_vertex_neighbours seems inefficient.

I agree there's room for some small optimizations here. However:

  1. That would be oustide the scope of a doc-focused PR
  2. The best approach to pathfinding is offloading it to non-Python code such as Rust or C++

Should I add __slots__?

Probably not in this PR, and I'm not sure about doing so in general. I think our pathfinding code is worth considering for refactoring in general, but it needs some careful consideration on how to do it right. It could be better to put a warning label on it and say that it's a suboptimal prototyping tool, but I'm not yet sure.

arcade/paths.py Outdated Show resolved Hide resolved
arcade/paths.py Outdated Show resolved Hide resolved
arcade/paths.py Outdated Show resolved Hide resolved
arcade/paths.py Outdated Show resolved Hide resolved
arcade/paths.py Outdated Show resolved Hide resolved
arcade/paths.py Outdated Show resolved Hide resolved
arcade/paths.py Outdated Show resolved Hide resolved
arcade/paths.py Outdated Show resolved Hide resolved
arcade/paths.py Outdated Show resolved Hide resolved
arcade/paths.py Outdated Show resolved Hide resolved
gran4 and others added 2 commits May 10, 2023 16:51
Co-authored-by: Paul <36696816+pushfoo@users.noreply.github.com>
Co-authored-by: Paul <36696816+pushfoo@users.noreply.github.com>
@gran4
Copy link
Contributor Author

gran4 commented May 10, 2023

Well, I was asking:
get_vertex_neighbours seems inefficient.
Should I add slots?

not for this PR put in general.
I've already submitted a separate PR to add slots.

I'll change it to use the other style.

gran4 and others added 3 commits May 10, 2023 16:53
Co-authored-by: Paul <36696816+pushfoo@users.noreply.github.com>
@einarf
Copy link
Member

einarf commented May 18, 2023

There are still some unresolved conversation here. What is the status on that?

@gran4
Copy link
Contributor Author

gran4 commented May 27, 2023

There are still some unresolved conversation here. What is the status on that?

@einarf sorry for the delay. I have been busy with finals, but school is over so I have time now. The PR is ready for review.

@gran4 gran4 requested review from pushfoo and eruvanos May 27, 2023 21:42
@pvcraven pvcraven merged commit 2583a89 into pythonarcade:development Jun 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants