483 refactor search #494
483 refactor search #494
Conversation
There was a problem hiding this comment.
Thanks so much for fleshing this doc out. Over time this has become a sort of blind spot for us. ❤️
earthaccess/search.py
Outdated
| response = get_results(self, limit) | ||
|
|
||
| results = list( | ||
| DataCollection(collection, self._fields) for collection in response |
CONTRIBUTING.md
Outdated
| ``` | ||
| pytest tests/unit | ||
| ``` | ||
|
|
There was a problem hiding this comment.
There's a Makefile, so this should be make test.
There was a problem hiding this comment.
I get the following error if I use the make command
$ make test
bash scripts/test.sh
+ pytest tests/unit --cov=earthaccess --cov=tests --cov-report=term-missing -s --tb=native --log-cli-level=INFO
ERROR: usage: pytest [options] [file_or_dir] [file_or_dir] [...]
pytest: error: unrecognized arguments: --cov=earthaccess --cov=tests --cov-report=term-missing
inifile: None
rootdir: /Users/djnewman/Documents/code/earthaccess
make: *** [test] Error 4
There was a problem hiding this comment.
I suspect this may be a knock-on effect from your poetry problem mentioned elsewhere, such that pytest-cov didn't get installed.
Can you successfully run these commands, as given at the top of the guide (possibly with conda in place of mamba):
mamba env update -f ci/environment-dev.yml
mamba activate earthaccess-dev
poetry install
CONTRIBUTING.md
Outdated
| In order to run the following scripts, run the following, | ||
| ``` | ||
| pip install mypy | ||
| mypy --install-types | ||
| pip install ruff | ||
| ``` | ||
| Then run these scripts to ensure correctx format, style and convention: |
There was a problem hiding this comment.
These are already installed by poetry install as instructed earlier in this guide. Also, the commands below should be replaced with make lint format, which is what I've done in this PR (in review): #497
There was a problem hiding this comment.
In my experience, I was unable to get these things working post poetry install from a fresh build. If we want to consolidate on make <test|lint|format> I'm cool with that.
There was a problem hiding this comment.
What problem did you encounter, because this works for me, as given at the top of the file (although with conda, not mamba)?
conda env update -f ci/environment-dev.yml
conda activate earthaccess-dev
poetry install
CONTRIBUTING.md
Outdated
| 2. Create a new virtual environment with conda/virtualenv | ||
| 3. Activate environment and install earthaccess as editable source | ||
| 1. `pip install -e .` | ||
| 2. `poetry install` (note, if we use Poetry with conda we need to tell poetry to not use its own virtual environments) |
There was a problem hiding this comment.
I believe these are covered by the commands given earlier in the guide (not exactly these commands, but same result).
There was a problem hiding this comment.
In my experience, I was unable to get these things working post poetry install from a fresh build. I consulted with @betolink and he provided the above instructions after I couldn't get the unit tests to run
There was a problem hiding this comment.
The steps at the top of the guide work for me, so what specific steps did you follow and what error did you encounter?
There was a problem hiding this comment.
I wonder if we can put these instructions on a separate "If you are not using Poetry, here is another way of getting a dev environment". Ideally, we should have a "universal" bash script ala npm install dev that will take care of everything including virtual environment and dev installs. In any case hopefully this PR gets merged soon-ish and we can keep iterating on it!
|
@doug-newman-nasa, I'd like to get your lovely enhancements landed soon. Any chance you can do the following things, which should get past the current build failures?
Once you make those changes, there may be further failures, but those 2 items are all that are currently visible in the build failures. |
|
@doug-newman-nasa, it looks like you still need to commit an updated |
|
@doug-newman-nasa, I don't think you pulled in changes from |
|
I see a bunch of unresolved merge conflict markers in the files you changed. |
|
Now that I've taken a closer look at your PR, I have a few change requests that I'll submit once you resolve the conflict markers. |
into a single function that utilizes Search-After
Updated documentation for linting.
using page_number parameter and were using CMR-Search-After header appropriately
Co-authored-by: Chuck Daniels <[email protected]>
Co-authored-by: Chuck Daniels <[email protected]>
Co-authored-by: Chuck Daniels <[email protected]>
Co-authored-by: Chuck Daniels <[email protected]>
Co-authored-by: Joseph H Kennedy <[email protected]>
the actual fix added. I suggest we find someone to install from scratch and document any issues with the instructions as they go along.
resolve issues with EDL and multiple people running tests.
doug-newman-nasa
force-pushed
the
483_refactor_search
branch
from
ecb6484 to
b22a660
Compare
Recorded requests due to change
|
@doug-newman-nasa, I'm seeing these test failures ( |
|
I thought @doug-newman-nasa had fixed those by ignoring the authentication headers and session cookies, is that not there Doug? +1 on making the tests less verbose, perhaps only be verbose on failures? (thinking VCR) |
level to 'error'
from request evaluation in VCR. pre-commit file updates
|
@betolink, Doug and I discovered that User-Agent and Accept-Encoding also had to be ignored. Doug also reduced vcr's verbosity and ran pre-commit locally, so hopefully that's that last of the build errors solved. |
📚 Documentation preview 📚: https://earthaccess--494.org.readthedocs.build/en/494/