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

io: getfile #939

Merged
merged 26 commits into from
Feb 28, 2025
Merged

io: getfile #939

merged 26 commits into from
Feb 28, 2025

Conversation

perazz
Copy link
Member

@perazz perazz commented Feb 21, 2025

This PR is spun off #904 for discussion with #919.

The present option provides a getfile function that has a similar functionality as the existing getline from the stdlib_io module:

stdlib_io

  • type(string_type) function getfile(fileName [,err] [,delete]) loads a whole ASCII file into a string_type variable all at once, with option to return a state flag, or to request file deletion (delete=.true.) after loading.

  • The file is loaded all at once for maximum computational efficiency.

  • The new state_type error handler is optionally used to provide clear error information. Its optional as commonly done in linear algebra routines: if provided, an error message may be returned; if not provided, exceptions trigger an error stop

cc: @jvdp1 @chuckyvt @jalvesz @fortran-lang/stdlib

@perazz perazz marked this pull request as ready for review February 21, 2025 11:19
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 @perazz. LGTM!
I am not a fan of the Camel case, but it is just a matter of taste.

perazz and others added 2 commits February 21, 2025 18:16
Co-authored-by: Jeremie Vandenplas <[email protected]>
Co-authored-by: Jeremie Vandenplas <[email protected]>
@chuckyvt
Copy link
Contributor

chuckyvt commented Feb 22, 2025

My suggestion is this is a better fit as a subroutine. I wouldn't expect anyone to use this in line with other operations. Also a subroutine is easier to extend to other data types in the future if desired. (Since the output argument and type is provided)

@perazz
Copy link
Member Author

perazz commented Feb 22, 2025

  • make it a subroutine
  • character(:), allocatable or string_type output interface

@chuckyvt
Copy link
Contributor

Well, while @perazz is making changes, any chance we could add a "notabs" type option? My organization loves to sprinkle tabs into text files, which often wrecks havoc downstream.

@perazz
Copy link
Member Author

perazz commented Feb 23, 2025

@chuckyvt the string module has a replace_all function that I believe could be a better way to handle that? As we load the whole file at once, there's no way to replace tabs with spaces withouth additional allocations anyway, so it makes sense to call a second function I guess?

Copy link
Contributor

@jalvesz jalvesz left a comment

Choose a reason for hiding this comment

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

Thanks @perazz for this contribution. Just a style comment, as @jvdp1 mentioned camel case is not recommended in the stdlib guidelines, actually here https://github.com/fortran-lang/stdlib/blob/HEAD/STYLE_GUIDE.md#variable-and-procedure-naming is mentioned that snake_case should be prefered. If possible, I would suggest applying it to the naming of the procedures get_file instead of getfile and so on.

@perazz
Copy link
Member Author

perazz commented Feb 25, 2025

Thank you for the edits @jalvesz - regarding the function name, I think it would not be a good idea to have get_file but getline. We could do get_file and get_line, at the price of a backward incompatible change. Open to discussion on this.

@jalvesz
Copy link
Contributor

jalvesz commented Feb 25, 2025

We could do get_file and get_line, at the price of a backward incompatible change. Open to discussion on this.

I would be in favor of this, considering that the interfaces are still tagged as experimental breaking changes are expected (we should document it). My reading here is that since there is no mechanism implemented to check the respect of the style guide, deviations have been introduced. I wonder if something like that could be done?

Out of curiosity I asked chatgpt: https://chatgpt.com/share/67bdeee7-a7dc-800f-bf12-6d9bf8774486 it looks feasible... there are many errors but the raw idea seems to hold, and the key point about "pre-commit" seems to be indeed available, maybe the tooling proposed by Codee could help? https://fortran-lang.discourse.group/t/webinar-enforcing-fortran-coding-guidelines-with-codee/9179

@jvdp1
Copy link
Member

jvdp1 commented Feb 25, 2025

would be in favor of this, considering that the interfaces are still tagged as experimental breaking changes are expected (we should document it).

I agree with @jalvesz; As it is in experimental it can still be changed. However, I don't have preferences between getline or get_line.

My reading here is that since there is no mechanism implemented to check the respect of the style guide, deviations have been introduced. I wonder if something like that could be done?

For my private projects in gitlab, I use fprettify to check the formatting. If changes are generated by fprettify, then the rest of the CI/CD does not start.

@perazz
Copy link
Member Author

perazz commented Feb 28, 2025

With two approvals, I will merge this one. Thank you @jvdp1 @jalvesz for the reviews.

@perazz perazz merged commit 69eaa20 into fortran-lang:master Feb 28, 2025
14 checks passed
@perazz perazz deleted the getfile branch February 28, 2025 10:05
@perazz perazz restored the getfile branch March 3, 2025 07:22
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