Optval

[fiolj]

Used template and added complex support and tests to optval

Note: now it seems to work. It is built after changes on common.fypp added in pull request #138 (Added complex to io)

[nshaffer]

Looks pretty solid to me! Only a few minor things stick out in this PR:

1. Some of the optval tests seemed mislabeled. I think it's helpful when testing templated routines to have the test name match the generated name as closely as possible.
2. No "test" for complex(sp) loadtxt written. The loadtxt tests hardly count as proper tests, in my opinion, but if you're adding complex(dp), might as well do the single-precision one as well, I think.

Let me know if it all makes sense.

[certik]

Thanks for extracting the optval function into fypp! We definitely want that.

Can you please not do any formatting changes in the same PR? If you want to propose formatting changes, please send a new PR with just those changes. If you could split the formatting changes from this PR, then we can merge it.

(We discussed this already elsewhere --- the formatting changes pollute the PR, so it's lots of unrelated changes that are hard to review if something changed, or if it's just a formatting change.)

[fiolj]

Ok, I'll roll back formatting, correct along the lines commented by @certik and @nshaffer and send new PRs for both #138 and #139. El 2/2/20 a las 8:12 PM, Ondřej Čertík escribió:

…

***@***.**** requested changes on this pull request. Thanks for extracting the optval function into fypp! We definitely want that. Can you please not do any formatting changes in the same PR? If you want to propose formatting changes, please send a new PR with just those changes. If you could split the formatting changes from this PR, then we can merge it. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#139?email_source=notifications&email_token=AAOTPJJKUKOZJGQC545PBITRA5HOFA5CNFSM4KO2JI3KYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCT5LOJI#pullrequestreview-351975205>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAOTPJLFUJ67LBR3S6YK5ADRA5HOFANCNFSM4KO2JI3A>.

[fiolj]

I added it by mistake. I've not used cmake before. What did confuse me was that stdlib_experimental_io.f90 was there even when it is generated by fypp (and stdlib_experimental_io.fypp is also present a few lines above). El 2/2/20 a las 6:37 PM, nshaffer escribió:

…

***@***.**** commented on this pull request. ------------------------------------------------------------------------ In src/CMakeLists.txt <#139 (comment)>: > @@ -25,6 +26,7 @@ set(SRC stdlib_experimental_error.f90 stdlib_experimental_kinds.f90 stdlib_experimental_optval.f90 + stdlib_experimental_stats.f90 I don't believe this entry should be here, since |stdlib_experimental_stats.f90| is generated by |fypp|. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#139?email_source=notifications&email_token=AAOTPJKJOIEUQZ5P5U56LKLRA44KBA5CNFSM4KO2JI3KYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCT5J73A#pullrequestreview-351969260>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAOTPJKOXM2XAG46JWEFPVLRA44KBANCNFSM4KO2JI3A>.

[jvdp1]

These are nice improvements. Thank you.
I only look to the changes to optval since the other changes are related to another PR (#138; I guess #138 should be merged before this #139).

Note to anyone who would be happy to do it: a spec stdlib_experimental_optval.md must be added.

[jvdp1]

Same proposition here: _int8 ==> _iint8 and so on.

[nshaffer]

I added it by mistake. I've not used cmake before. What did confuse me was that stdlib_experimental_io.f90 was there even when it is generated by fypp (and stdlib_experimental_io.fypp is also present a few lines above).

No worries, really this should be automatically handled by CMake, it's just not in place yet.

[nshaffer]

With @jvdp1's naming suggestions and my little nitpick below, I think optval will be in good shape.

[nshaffer]

Note to anyone who would be happy to do it: a spec stdlib_experimental_optval.md must be added.

I have written a spec locally and will submit a PR sometime soon.

[certik]

I had to look at the diff locally after merging master into it, and it looks great. Thank you again for simplifying the code.