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

add fre dora tools #307

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Conversation

menzel-gfdl
Copy link
Contributor

Describe your changes

Adds functionality to interact with dora - (we should think about the best way to test this without polluting dora).
These commands will only work with a valid dora api token.

Issue ticket number and link (if applicable)

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

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.

looks in the right direction

idk what we can do to avoid polluting dora's database without including some kind of mock DB on dora's end to explicitly address testing cruft.

Copy link
Member

Choose a reason for hiding this comment

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

can use globs- e.g. fre/app/regrid_xy/tests/*

from subprocess import CalledProcessError
from tempfile import TemporaryDirectory

from analysis_scripts import available_plugins, UnknownPluginError
from fre.analysis.subtools import install_analysis_package, list_plugins, run_analysis
import pytest
Copy link
Member

Choose a reason for hiding this comment

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

@menzel-gfdl i'm gonna nit you here more for a good discussion on style regardless of impact. pylint doesn't seem to care, but shouldn't the module import order be, to steal C++ syntax for a minute:

<pystdlib> 
<3rd-party external modules>
<sub modules in this package>

all sub-sorted alphabetically of course, i can't unsee that now

Copy link
Contributor Author

@menzel-gfdl menzel-gfdl Jan 21, 2025

Choose a reason for hiding this comment

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

I always wonder the same thing too. I guess is the question is, as far as test_subtools.py is concerned, is fre a 3rd party module? I like to view the tests as outside of the package (thus the non-relative imports), but I don't know if I am correct about that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://peps.python.org/pep-0008/#imports

Is what I did compatible with this?

Copy link
Member

Choose a reason for hiding this comment

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

I always wonder the same thing too. I guess is the question is, as far as test_subtools.py is concerned, is fre a 3rd party module? I like to view the tests as outside of the package (thus the non-relative imports), but I don't know if I am correct about that.

This is exactly why I thought i'd stir the pot here- I had not thought of, and I agree, that the question to answer is really, "are the tests part of the package?" In fre-cli's case, for now at least, the tests are considered part of the package. If they're part of the package, then i think the text from the link applies:

"Imports should be grouped in the following order: 
 1. Standard library imports. 
 2. Related third party imports. 
 4. Local application/library specific imports."

so by that logic, analysis_scripts is third-party in this context... though perhaps this feels a little silly because we work on that package so closely. pytest is similarly third-party, but fre.analysis.subtools is the local application import.

just in case it's not clear, this is absolutely a nit and not a blocking issue whatsoever.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, thanks!

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.

2 participants