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 basics API reference #18

Merged
merged 2 commits into from
Oct 10, 2023
Merged

Add basics API reference #18

merged 2 commits into from
Oct 10, 2023

Conversation

SimonHeybrock
Copy link
Member

@SimonHeybrock SimonHeybrock commented Oct 4, 2023

This is not 100% complete as we are still working on a more user-friendly solution in some cases, but these are the most important bits.

This is not 100% complete as we are still working on a more
user-friendly solution in some cases, but these are the most important
bits.
:template: module-template.rst
:recursive:

beam_center_finder
Copy link
Member

Choose a reason for hiding this comment

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

Is there a way we can improve the long function names? Maybe only have the function name and not the submodule path? But I guess that path is important information to have...
Screenshot at 2023-10-04 15-16-28

Copy link
Member Author

Choose a reason for hiding this comment

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

I suppose we can shorten, beam_center_finder -> beam_center, or remove the duplicate beam_center_ prefix from the 2 finders?

Copy link
Member

Choose a reason for hiding this comment

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

remove the duplicate beam_center_ prefix from the 2 finders?

would work in this case. But I guess it won't save us from future functions with long names.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe having shorter module names is good style generally. Should we just rename beam_center_finder -> beam?

After all, PEP8 says: "Modules should have short, all-lowercase names. Underscores can be used in the module name if it improves readability."

Copy link
Member

Choose a reason for hiding this comment

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

Or beamcenter? I feel beam is a little too vague?

Copy link
Member Author

Choose a reason for hiding this comment

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

I suggest we keep it for now. We should probably sit down with Wojciech to review all the naming in the repo.


beam_center_finder
sans2d
types
Copy link
Member

@nvaytet nvaytet Oct 4, 2023

Choose a reason for hiding this comment

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

Was it intentional that some entries are under "Module attributes" while others are under "Classes"? I don't really seem to get the logic separating the two sections.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is not intentional, but this is what we get from the generic template, and due to the fact that some domain types are using NewType while others are not (are actual classes). I don't know how to fix that. Maybe the template could be rewritten?

@SimonHeybrock SimonHeybrock merged commit 27df9c8 into main Oct 10, 2023
3 checks passed
@SimonHeybrock SimonHeybrock deleted the api-reference branch October 10, 2023 04:41
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.

2 participants