Skip to content

224.fre make compile test#295

Merged
singhd789 merged 27 commits intomainfrom
224.fre-make-compile-test
Jan 21, 2025
Merged

224.fre make compile test#295
singhd789 merged 27 commits intomainfrom
224.fre-make-compile-test

Conversation

@singhd789
Copy link
Collaborator

@singhd789 singhd789 commented Dec 16, 2024

Describe your changes

Add create-compile test

Issue ticket number and link (if applicable)

#224

Checklist before requesting a review

  • I ran my code
  • I tried to make my code readable
  • I tried to comment my code
  • I wrote a new test, if applicable
  • I wrote new instructions/documentation, if applicable
  • I ran pytest and inspected it's output
  • I ran pylint and attempted to implement some of it's feedback

@singhd789 singhd789 requested a review from rem1776 December 26, 2024 17:43
@singhd789 singhd789 marked this pull request as ready for review December 26, 2024 17:43
Copy link
Contributor

@kiihne-noaa kiihne-noaa left a comment

Choose a reason for hiding this comment

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

looks good to me! We discussed a number of these changes last week.

@singhd789 singhd789 requested a review from Ciheim December 26, 2024 17:56
@singhd789 singhd789 requested a review from ilaflott January 7, 2025 22:13
rem1776
rem1776 previously requested changes Jan 8, 2025
Copy link
Member

Choose a reason for hiding this comment

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

why click.echo instead of print or logger ?

Copy link
Collaborator Author

@singhd789 singhd789 Jan 13, 2025

Choose a reason for hiding this comment

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

Good question. I believe Bennett made it click.echo, which was a cool thing to keep with click functionalities. I think I read somewhere that it might've been preferred (vs print) so I kept it in


# Check for creation of compile script, FMS directory,
# log.compile file, the executable
assert Path(f"{OUT}/fremake_canopy/test/null_model_full/{plat}-{targ}/exec/compile.sh").exists()
Copy link
Member

Choose a reason for hiding this comment

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

f"{OUT}/fremake_canopy/test/null_model_full/{plat}-{targ}/exec/" could definitely be assigned to its own variable somewhere. unless you wanna edit the path in several places later upon needed to extend/fix something

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the platforms and targets sometimes change (test for bad platforms and targets or multi)

Copy link
Member

Choose a reason for hiding this comment

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

sometimes functions like this:

some_func(arg1_val, arg2_val, arg3_val ...)

are nicer when written out like this:

some_func(arg1 = arg1_val, 
          arg2 = arg2_val, 
          arg3 = arg3_val ...)

Copy link
Member

@ilaflott ilaflott left a comment

Choose a reason for hiding this comment

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

it works, if needing some cleanup. Granting approval, but the only clarification i'm really wondering: why click.echo in create_compile_script ?

@ilaflott ilaflott dismissed rem1776’s stale review January 17, 2025 22:54

the requested change was addressed and i resolved the feedback- not sure why this is still blocked based on that alone.

@singhd789 singhd789 merged commit a9730e0 into main Jan 21, 2025
2 checks passed
@singhd789 singhd789 deleted the 224.fre-make-compile-test branch January 21, 2025 19:53
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

Comments