Skip to content

Conversation

@VincentVanlaer
Copy link
Member

No description provided.

Whether this is the better choice vs keeping all function pointers set
to null is potentially a future debate
@Debraheem
Copy link
Member

Is this just a simplification so that users can call the public star_data functions for making a pgstar decorator, as opposed to using a null other_* function? Is this easy for a random user to do? We generally point users to use the other_* functions, so we just want it to be clear how one adds their own decorator. I'm not sure if it's good or bad to do this. We don't want to hide this from users, and I don't know anyone that looks inside star_data/public, but maybe we can advertise it somewhere?

@VincentVanlaer
Copy link
Member Author

I am not sure I fully understand your question? The only reason for the null functions to exist would be if they are used for initializing hooks. For the pgplot hooks in star and binary these are just initialized to null.

If you mean that these serve as an example for how to implement the specific hook, the interface definitions are basically the same. So perhaps that needs to be documented more clearly then?

@Debraheem
Copy link
Member

Yes, my apologies, I mean the latter. I was just worried by the hook not being visible in star/other, but i guess it's a non issue.

We just want the interface format to be advertised somewhere in the docs, or mentioned. You've basically already done that, but maybe add the null interface example in $MESA_DIR/binary/public/binary_pgbinary.f90 as well.

Otherwise these changes look fine to me.

@warrickball
Copy link
Contributor

I've come back to this because I keep deleting the use pgstar_decorator line to build without PGSTAR, and it would be good to resolve. But I basically never use PGSTAR, and have certainly never created custom decorators, so I don't feel qualified to comment. Perhaps we just need to know:

  • How would users currently use the things we're moving/removing/changing?
  • How would they use them after this change?
  • Does it remain clear how to use whatever features are affected?

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.

4 participants