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

fix : convert trailing comments to single sline comments #711

Merged
merged 6 commits into from
Sep 8, 2023

Conversation

gaelforget
Copy link
Contributor

@gaelforget gaelforget commented Jul 20, 2023

Seems safer since fewer parsers will support comments trailing section lines than single line comments.

With at least one parser, IniFile.jl, these comments lead to an incorrect read -- separating out the comments as single lines fixed it.

@bpbond
Copy link
Member

bpbond commented Jul 24, 2023

Thanks @gaelforget . What's the use case for this? I.e., why would another parser be reading Hector's INI files?

@codecov-commenter
Copy link

Codecov Report

Merging #711 (32d85eb) into master (8087ca7) will not change coverage.
The diff coverage is n/a.

❗ Current head 32d85eb differs from pull request most recent head d00c381. Consider uploading reports for the commit d00c381 to get more accurate results

@@           Coverage Diff           @@
##           master     #711   +/-   ##
=======================================
  Coverage   79.72%   79.72%           
=======================================
  Files          61       61           
  Lines        5978     5978           
=======================================
  Hits         4766     4766           
  Misses       1212     1212           

@gaelforget
Copy link
Contributor Author

Thanks @gaelforget . What's the use case for this? I.e., why would another parser be reading Hector's INI files?

Use case is running Hector via a Julia wrapper. I do this in the following notebook that is part of the example set for ClimateModels.jl (the generalized model wrapper).

http://www.gaelforget.net/notebooks/Hector.html

Similar to what's done in pyhector and the R interface I presume, we read in the ini file to display parameters, let user change parameters, save customized parameters to a new ini file, and call Hector from Julia.

@bpbond bpbond requested a review from kdorheim July 24, 2023 14:01
@kdorheim
Copy link
Contributor

kdorheim commented Aug 9, 2023

@gaelforget, thanks for your interest in contributing to Hector! I guess I am curious as to why this change was only made for the HFC4310_halocarbon?

@gaelforget
Copy link
Contributor Author

Other comments were single line comments or after variable definitions & did not seem to cause any issue -- unlike these few comments placed after section definitions ([HFC4310_halocarbon ]).

@kdorheim kdorheim changed the base branch from master to dev August 14, 2023 20:11
@kdorheim
Copy link
Contributor

kdorheim commented Sep 8, 2023

This does not affect our miscellaneous scripts that are not tested as part of the CI (this is a todo #657) good to merge!

@kdorheim kdorheim merged commit c452322 into JGCRI:dev Sep 8, 2023
9 of 10 checks passed
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.

5 participants