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

style: Advertise underscore_separation for function names #265

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

zhaohany
Copy link

This PR updates the naming conventions section to promote the use of underscore_separation for function names, in alignment with [PEP 8](https://pep8.org/) guidelines.

Fixes issue: [UCL/rsd-engineeringcourse#263](#263)

Copy link
Member

@Saransh-cpp Saransh-cpp left a comment

Choose a reason for hiding this comment

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

Thanks, @zhaohany! Just a minor suggestion -

ch05construction/02conventions.ipynb.py Outdated Show resolved Hide resolved
Co-authored-by: Saransh Chopra <[email protected]>
Copy link
Member

@Saransh-cpp Saransh-cpp left a comment

Choose a reason for hiding this comment

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

Thanks for picking this up!

@zhaohany
Copy link
Author

Thank you @Saransh-cpp, seems the build failed due to a key, I see similar PR before can skip this steps but I do not have access to trigger the build, would you please help merge this? Thank you.

Copy link
Member

@Saransh-cpp Saransh-cpp left a comment

Choose a reason for hiding this comment

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

Hi, @zhaohany! Please don't worry about the CI. The CI is failing because this PR is from a fork. A minor comment that I missed before -

Comment on lines -135 to -143
# %% [markdown]
# This other example uses underscore_separation for all the names.

# %%
class class_name:
def method_name(a_variable):
m_instance_variable = a_variable


Copy link
Member

Choose a reason for hiding this comment

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

I would keep this as it is (a possible but not a recommended use-case).

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