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

using context managers to write files #120

Closed

Conversation

shimwell
Copy link
Collaborator

@shimwell shimwell commented May 1, 2024

This PR changes the write statements to use context managers. This removes the need for a .close() when the file writing is done.

Context managers do however require another indentation which means the lines are shorter for the code below. The formatter has therefore decided that there is a more optimal layout for these lines, now that they have less space to stay below the line limit. So we have a bigger diff than might be expected when starting the PR

Requested in issue #111

closes #111

@alberto743 are you ale to review

tests passing locally

@alberto743
Copy link
Member

@shimwell Yes, I can review by next Monday 6th.

@psauvan psauvan requested a review from alberto743 May 3, 2024 12:31
@psauvan psauvan assigned psauvan and alberto743 and unassigned psauvan May 3, 2024
@alberto743
Copy link
Member

Hi @shimwell . Thank you for the development, which is generally fine for me.
Here are a few comments related to a problem already present in the lines you have changed.

  1. the self.inpfile should be transformed to a local variable, which needs to be passed cross function calls, because logically the other private methods of the MONTECARLOCODENAMEInput class should not depend upon an attribute not initialized by the __init__ method. Also, trailing underscores in methods should be avoided. The issue was indeed already present in the previous code.
  2. testing/test.py should be changed at lines 55-57 as well
  3. geouned/GEOUNED/Utils/BasicFunctions_part2.py has the same problem at line 18, but maybe in this case it would be better to open a separate logging instance instead of writing to a file
  4. geouned/GEOUNED/__init__.py presents the issue in function print_warning_solids, but maybe this one as well may be transformed to a logging instance
  5. geouned/GEPOReverse/Modules/Parser/parser.py still has dangling open calls between lines 1214-1253, but maybe this section related to Python 2.7 shall be simply stripped away since it is dead code because of line 13

@shimwell
Copy link
Collaborator Author

shimwell commented May 9, 2024

Thanks alberto

I shall get this branch merge compatible with dev and get started on your recommendations. Much appreciated

@shimwell shimwell marked this pull request as draft May 9, 2024 18:08
@shimwell
Copy link
Collaborator Author

@alberto743 I've done 4 out of the 5 suggestions. I'm just holding back on the first suggestion regarding self.inpfile which was present prior to this PR. The reasons I'm seeing a few methods such as write_xml and write_py both write files it gets a bit messy. I'm thinking it might be best to have each of these smaller functions in the OpenMCFormat return a string or list of stings and then combine the strings and write it all in a single write function instead of passing around an open file handle and continually writing to it. I would prefer to do this in another PR if that is ok.

@shimwell shimwell marked this pull request as ready for review May 10, 2024 15:34
@shimwell
Copy link
Collaborator Author

shimwell commented May 10, 2024

Hi @shimwell . Thank you for the development, which is generally fine for me. Here are a few comments related to a problem already present in the lines you have changed.

  1. the self.inpfile should be transformed to a local variable, which needs to be passed cross function calls, because logically the other private methods of the MONTECARLOCODENAMEInput class should not depend upon an attribute not initialized by the __init__ method. Also, trailing underscores in methods should be avoided. The issue was indeed already present in the previous code.

Not done yet but would like to fix this in a follow up PR. self.inpfile was also not introduced by this PR so perhaps a bit of topic to fix it now.

  1. testing/test.py should be changed at lines 55-57 as well

Done

  1. geouned/GEOUNED/Utils/BasicFunctions_part2.py has the same problem at line 18, but maybe in this case it would be better to open a separate logging instance instead of writing to a file

Done as a logging instance

  1. geouned/GEOUNED/__init__.py presents the issue in function print_warning_solids, but maybe this one as well may be transformed to a logging instance

Done as a logging instance

  1. geouned/GEPOReverse/Modules/Parser/parser.py still has dangling open calls between lines 1214-1253, but maybe this section related to Python 2.7 shall be simply stripped away since it is dead code because of line 13

Done by removing old python 2.7 code

@alberto743
Copy link
Member

@alberto743 I've done 4 out of the 5 suggestions. I'm just holding back on the first suggestion regarding self.inpfile which was present prior to this PR. The reasons I'm seeing a few methods such as write_xml and write_py both write files it gets a bit messy. I'm thinking it might be best to have each of these smaller functions in the OpenMCFormat return a string or list of stings and then combine the strings and write it all in a single write function instead of passing around an open file handle and continually writing to it. I would prefer to do this in another PR if that is ok.

Fully agree!

@shimwell
Copy link
Collaborator Author

shimwell commented May 14, 2024

closing and replacing with PR #160 as I accidently made some commits to this branch and messed it up

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.

3 participants