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

Better file organization #98

Open
Jashcraf opened this issue Nov 8, 2023 · 1 comment
Open

Better file organization #98

Jashcraf opened this issue Nov 8, 2023 · 1 comment

Comments

@Jashcraf
Copy link
Owner

Jashcraf commented Nov 8, 2023

Recently added poke.materials which loads index data in c38ffe6. However the test on GH actions broke because I was supplying it the wrong path? Even though it works locally.

This is what we call in the industry a "skill issue". Maybe this can be @kenjim21's next project

@kenjim21 what do you think of helping me have a better filepath management system that enables better unit tests?

@brandondube
Copy link

FWIW, the way I did prysms sample data is invariant to where the code is run

  1. Put the files in a deterministic place that won't change - https://github.com/brandondube/prysm/tree/master/sample_files
  2. Make a small bit of logic that goes and downloads them, if they are not already local, and handles the paths - https://github.com/brandondube/prysm/blob/master/prysm/sample_data.py
  3. Use pathlib because pathlib is love, pathlib is life

The gist of why I did the dynamic download thing is because the sample files are ~10MB, and prysm's source was at the time ~100kb, and I did not want to pollute the source distribution that goes on PyPI.

If your material files are essential, then the dynamic download wouldn't really make much sense (but would allow you to decouple the updating of material data from software versioning, if you wanted to).

What maybe is, or likely is your problem, is a difference in operating system / file system between your local development and the continuous integration (Ubuntu, from a test log).

When you slice up to -12 on line 7 of materials.py, that is potentially error-prone; you could get a trailing slash or not, depending on silly things like non-printing control characters, or double slashes on some platforms, etc. It would be better to write that as Path(__file__).parent/'material_data'). You could also do .parent.absolute() if you want to replicate the absolute portion of that logic, though I think __file__ is always absolute. With os.path, you would want to replace fullpth = matfilepath+'material_data/'+file by fullpth = path.join(matfilepath,'material_data',file). This will let os.path take care of all of the slashes for you.

Basically, don't try to manipulate paths as strings; use a path library (and path lib is the "modern" one that is like 10+ years newer than os.path and less crusty)

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

No branches or pull requests

2 participants