Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Expose Parameter Handler Tag Name #662

Closed
SimonBoothroyd opened this issue Jul 27, 2020 · 7 comments
Closed

Expose Parameter Handler Tag Name #662

SimonBoothroyd opened this issue Jul 27, 2020 · 7 comments

Comments

@SimonBoothroyd
Copy link
Contributor

Is your feature request related to a problem? Please describe.
It would be useful to publicly expose the ParameterHandler._TAGNAME attribute so there is a programatic way to access this for a given class of handler. This would enable e.g.:

force_field.get_parameter_handler(ChargeIncrementModelHandler.TAGNAME)

Describe the solution you'd like
Expose ParameterHandler._TAGNAME as ParameterHandler.TAGNAME or similar.

@mattwthompson
Copy link
Member

I've found myself doing this as well (https://github.com/openforcefield/openff-system/search?q=_TAGNAME&unscoped_q=_TAGNAME) and agree that exposing it would be useful. Maybe as ParameterHandler.handler_name or something because the noun "tag" is not used much, if at all, in the public API. Any grounds for hesitation here @j-wags? Otherwise I will implement this

@j-wags
Copy link
Member

j-wags commented Jul 28, 2020

This sounds good, though I might stick with exposing it as ParameterHandler.TAGNAME. The "Tag" is from the XML element nomenclature. It's a more XML-centric word than I'd like, but it's the reality of the current situation.

Also, I don't see significant harm in exposing a setter for it as well. So let's go ahead and expose that.

@mattwthompson
Copy link
Member

I'd push for .tag_name to be consistent with PEP8 naming style but perhaps the value of consistency between the internal and exposed attributes overrides that?

@j-wags
Copy link
Member

j-wags commented Jul 28, 2020

I'd push for .tag_name to be consistent with PEP8 naming style but perhaps the value of consistency between the internal and exposed attributes overrides that?

Good point, let's do tag_name.

Also, I don't see significant harm in exposing a setter for it as well. So let's go ahead and expose that.

Ah, I'm a fool. This is a class attribute, not an instance attribute. So let's not expose the setter.

@mattwthompson
Copy link
Member

I totally overlooked that as well (and, to be honest, had to do some digging for why that's a problem!)

@SimonBoothroyd
Copy link
Contributor Author

I'd push for .tag_name to be consistent with PEP8 naming style

I think for class constants the PEP8 style is all capitals - so probably TAG_NAME

mattwthompson added a commit that referenced this issue Aug 5, 2020
@j-wags
Copy link
Member

j-wags commented Aug 13, 2020

Closed by #665

@j-wags j-wags closed this as completed Aug 13, 2020
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants