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 utility function to generate property list #40

Closed
wants to merge 6 commits into from

Conversation

stefanbringuier
Copy link

Problem

A frequent operation that is carried out using PeriodicTable.jl is to generate a list of element properties from Elements. This pull request adds a simple utility function to the src/PeriodicTable.jl module.

Solution

A new function getlist which returns a list when passed a field name variable (::Symbol or ::String) of Element which is in Elements.

The following are example method calls:

getlist(elements,:name)
...
getlist(elements[1:10],"symbol")
...
getlist(elements[[6,74]],:density)
...

Comments

The function/methods have been placed in PeriodicTable.jl module and exported from within that scope, but it may be more practical to put this in a submodule file Utilities.jl if more utility functions are added in the future.

Add utility functions to return list of passed `Element` field from `Elements`.
Additional method if a Vector{Element} is passed, i.e., slice of elements.
Add module export of `getlist`.
Add tests for utility function.
@carstenbauer
Copy link
Member

carstenbauer commented Nov 3, 2021

First of all, thanks for the PR!

However, I fail to see the actual advantage of having a dedicated getlist given that one can just use basic Julia features (broadcasting, getproperty / getfield) to achieve the same:

julia> getproperty.(elements, :name) == getlist(elements, :name)
true

julia> getproperty.(elements[[6,74]], :density) == getlist(elements[[6,74]],:density)
true

The only new "feature" here that I can see is support for Strings instead of Symbols which, IMHO, isn't worth it.

(One can't even really make the argument that accessing properties / fields of elements is bad style - because it could be an implementation detail that isn't part of the API - given that we explicitly document their existence and encourage their usage in the README)

But let's see what others think.

@stefanbringuier
Copy link
Author

@carstenbauer thanks for the consideration. You're completely correct about this providing no true advantage, its merely for convenience and style. I was just looking to avoid broadcasting notation and add String support. Feel free to reject/close if no additional interest.

@carstenbauer
Copy link
Member

I'll close this for now.

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