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

Lexicon update for the pybind module. #73

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

NurAd-Din
Copy link

This is the first pull request I'm making for the new python bindings. This is also the first pull request I've ever made so please give some guidance on whether I wish add more content per pull request. This request includes some smaller classes relating to the major lexicon class exposure.

@NurAd-Din NurAd-Din self-assigned this Jul 22, 2024
@JackTemaki
Copy link
Contributor

This is also the first pull request I've ever made so please give some guidance

Sure, here some hints:

  • change the title to reflect the actual changes / additions of your PR
  • Change the description text and add a few more details what you are actually adding, although in this case there might be no useful information
  • regarding the PR size: this is usually hard to determine. I would say here the addition is quite small, so if there is more useful things to add to that, I think you can make the PR a little larger. Just try to have this in blocks which cover related logic. As I am not familiar with RASR internals, it is hard for me to give guidance on what should be added at the same time.

src/Tools/LibRASR/PybindModule.cc Outdated Show resolved Hide resolved
src/Tools/LibRASR/PybindModule.cc Outdated Show resolved Hide resolved
src/Tools/LibRASR/PybindModule.cc Show resolved Hide resolved
src/Tools/LibRASR/PybindModule.cc Outdated Show resolved Hide resolved
src/Tools/LibRASR/PybindModule.cc Show resolved Hide resolved
src/Tools/LibRASR/PybindModule.cc Outdated Show resolved Hide resolved
@NurAd-Din NurAd-Din changed the title Content for first pull request. Lexicon update for the pybind module. Jul 31, 2024
@NurAd-Din
Copy link
Author

I've changed the request to include all Lexicon class exposure for approval. I've also implemented some of the requested changes.

@hannah220
Copy link

Some class has a single default constructor .def(py::init<>()), some don't have a constructor at all.
If the C++ class has an implicit or explicitly defined default constructor (e.g., no arguments and does nothing special), and no other constructors, pybind11 will automatically expose it, and you don’t need to define .def(py::init<>()).
Can you make it consistent by only defining .def(py::init<>()) when it's necessary?

src/Tools/LibRASR/PybindModule.cc Show resolved Hide resolved
src/Tools/LibRASR/PybindModule.cc Show resolved Hide resolved
src/Tools/LibRASR/PybindModule.cc Outdated Show resolved Hide resolved
@NurAd-Din
Copy link
Author

Previously requested changes implemented.

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