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

Made format constants public #617

Merged
merged 12 commits into from
Jan 18, 2022
Merged

Conversation

lewisfish
Copy link
Contributor

Hey,
This resolves #570.
Pull request makes the format constants from stdlib_io public.

Let me know if I need to do anything else as this is my first pull request, so not 100% sure of the protocol.

Lewis

@awvwgk
Copy link
Member

awvwgk commented Jan 12, 2022

Thanks for starting this. Have a look at https://github.com/fortran-lang/stdlib/blob/HEAD/doc/specs/stdlib_io.md to add a description of the constants which should become part of the public API.

@awvwgk
Copy link
Member

awvwgk commented Jan 12, 2022

Another point for discussion is how the format specifier should be exported. Do we want to be able to use it in a context like this:

use stdlib_io, only : FMT_REAL_SP, FMT_COMPLEX_SP
implicit none

print '('//FMT_REAL_SP//',1x,'//FMT_COMPLEX_SP//')', 1.0, (2.0, 3.0)
end

I wonder whether the *(...) repeat will mess this up.

Further, what about non-advancing output, some format specifies have a trailing skip (1x for int and real), other don't (complex). Should we include an explicit break for such case, i.e. (es24.16e3,:,1x)?

@lewisfish
Copy link
Contributor Author

Thanks for starting this. Have a look at https://github.com/fortran-lang/stdlib/blob/HEAD/doc/specs/stdlib_io.md to add a description of the constants which should become part of the public API.

Added now, let me know if you have any comments on the format of this as wasn't too sure what to copy format-wise from the other examples in the docs.

I wonder whether the *(...) repeat will mess this up.

Further, what about non-advancing output, some format specifies have a trailing skip (1x for int and real), other don't (complex). Should we include an explicit break for such case, i.e. (es24.16e3,:,1x)?

Is there any reason the complex doesn't have the trailing skip? It would make more sense if all the formats had the same output

@milancurcic
Copy link
Member

Good points, I think we should export constants without the *(...) and the 1xs, and let the user adapt them for arrays if needed.

Is there any reason the complex doesn't have the trailing skip? It would make more sense if all the formats had the same output

I think I implemented this and I don't remember why it is like this. It's possible that I did it by mistake and the tests were insufficient to make it an issue.

@lewisfish
Copy link
Contributor Author

Good points, I think we should export constants without the *(...) and the 1xs, and let the user adapt them for arrays if needed.

Is there any reason the complex doesn't have the trailing skip? It would make more sense if all the formats had the same output

I think I implemented this and I don't remember why it is like this. It's possible that I did it by mistake and the tests were insufficient to make it an issue.

Great, I'm happy to implement this.
save_txt uses these constants, the easiest way I see to implement this would be to remove the * and 1x from the format constants then concat them in the required places in save_txt. Does that make sense?

@milancurcic
Copy link
Member

save_txt uses these constants, the easiest way I see to implement this would be to remove the * and 1x from the format constants then concat them in the required places in save_txt. Does that make sense?

That seems fine to me. 👍

@lewisfish
Copy link
Contributor Author

Okay, updated the code and docs. Let me know if anything needs changed or adjusted.

Copy link
Member

@milancurcic milancurcic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

Copy link
Member

@awvwgk awvwgk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for sharing. Looks good to me.

Could you add a note about this change in https://github.com/fortran-lang/stdlib/blob/HEAD/CHANGELOG.md as well?

@lewisfish
Copy link
Contributor Author

Thanks for sharing. Looks good to me.

Could you add a note about this change in https://github.com/fortran-lang/stdlib/blob/HEAD/CHANGELOG.md as well?

Done. Thanks for all your and @milancurcic help in getting this done!

Copy link
Member

@jvdp1 jvdp1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you.
Small questions: is there no tests?
Also, I think that these will not be documented in Ford docs. Could you add some comments for each format (or at least one general comment) with a link to the specs, please?

@lewisfish
Copy link
Contributor Author

Thank you. Small questions: is there no tests? Also, I think that these will not be documented in Ford docs. Could you add some comments for each format (or at least one general comment) with a link to the specs, please?

With regards to tests. There are no explicit tests. However, there are some implicit ones within the save_txt tests since save_txt uses the format specifiers. Is that sufficient?

Added some comments for each specifier. Let me know if I've done the documentation wrong as I've not used FORD before.

Copy link
Member

@jvdp1 jvdp1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @lewisfish . It looks good to me!

Co-authored-by: Jeremie Vandenplas <[email protected]>
@milancurcic
Copy link
Member

Thank you @lewisfish and reviewers!

@milancurcic milancurcic merged commit 084f3c4 into fortran-lang:master Jan 18, 2022
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.

Provide precision-preserving edit descriptors
4 participants