NOAA-GFDL / fre-cli

Python-based command line interface for FRE (FMS Runtime Environment) to compile and run FMS-based models and post-process their output.
GNU Lesser General Public License v3.0
3 stars 7 forks source link

Remove extra `write` for `c[additional instructions]` #123

Closed singhd789 closed 1 month ago

singhd789 commented 1 month ago

Describe your changes

Remove self.checkoutScript.write(c['additionalInstructions']) because this step was done previously. This line was redundant and was causing additionalInstructions to be written one more time than needed in the checkout script that is created.

Issue ticket number and link (if applicable)

N/A

Checklist before requesting a review

ilaflott commented 1 month ago

Doesn't seem redundant... the docs step is now broken. Can you elaborate on why this additionalInstructions writing step is redundant?

singhd789 commented 1 month ago

@ilaflott the additionalInstructions are per component in the compile yaml for fremake. They are being written for if there are multiple repos https://github.com/NOAA-GFDL/fre-cli/blob/a6c744c89be82a04cdb8eb7cd8dbdb6c16956385/fre/make/gfdlfremake/checkout.py#L76 and for if there are not multiple repos https://github.com/NOAA-GFDL/fre-cli/blob/a6c744c89be82a04cdb8eb7cd8dbdb6c16956385/fre/make/gfdlfremake/checkout.py#L78 already.

So the next write line https://github.com/NOAA-GFDL/fre-cli/blob/a6c744c89be82a04cdb8eb7cd8dbdb6c16956385/fre/make/gfdlfremake/checkout.py#L80 is not needed, as it would just paste the same additionalInstructions that were defined per component, and already written.

singhd789 commented 1 month ago

@ilaflott maybe I misunderstood something about the documentation process? Let me try to see what I can find

singhd789 commented 1 month ago

One difference I see is that the last sphinx build for docs (last pr) was using: Run sphinx-build docs build Running Sphinx v7.3.7

Meanwhile, this pr is using: Run sphinx-build docs build Running Sphinx v7.4.4

ilaflott commented 1 month ago

OK fair- your proposed change seems totally unrelated to the doc failure, i don't know what that's about.

ilaflott commented 1 month ago

is it possible to re-factor this functions logic a little bit?

one could argue the redundant line is there because what the function is accomplishing isn't super clear.