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

Added static file folder #41

Merged
merged 2 commits into from
Feb 23, 2024
Merged

Conversation

menamerai
Copy link
Collaborator

Is there a way to default to using the delphi/static folder instead of the delphi folder from the from importlibs.resources import files @jaidhyani ? In here I just join the path with static every time.

@menamerai menamerai requested a review from jaidhyani February 19, 2024 21:47
@menamerai menamerai linked an issue Feb 19, 2024 that may be closed by this pull request
@jaidhyani
Copy link
Collaborator

Sort of - files takes a module (or string-specifying-a-module) as an argument. So to make it point somewhere other than our package, we would need to give it a different argument. One thing we could do would be to add an __init__.py to the static dir, and then we could do files(delphi.static). Either approach is fine, I think - your call.

But either way, you're right that we shouldn't require people to type that out each time. Let's add a delphi/constants.py, which for now can just contain STATIC_ASSETS_DIR definition.

@menamerai menamerai marked this pull request as ready for review February 20, 2024 03:47
@menamerai
Copy link
Collaborator Author

@jaidhyani I added __init__.py to the directory, and added the constants file. How does it look now?

Copy link
Collaborator

@jaidhyani jaidhyani left a comment

Choose a reason for hiding this comment

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

This looks good to me. Minor follow-up tasks (edit: added as issue #42). These aren't blockers, code can be merged

  1. We shouldn't necessarily force the output into the STATIC_ASSETS_DIR. Especially if the user specified a full path or a partial path with directory references, e.g.:

/home/me/Desktop/tokens.csv
subdir/tokens.csv

In the first case, the user has specified a full path, and we should use it. In the second, we can assume they're referring to a relative path from the current working directory, and we should output there.

We can get that behavior by calling os.path.split on the path arg, which returns a (head,tail) tuple. If head is empty, the user just passed a filename and we can probably throw it in the default output dir. If head isn't empty, we can call os.path.expandvars on the path arg to get the path we should output to.

  1. We should also print the full path before writing, just to be explicit with the user about what file is being created and they know exactly where to look for it when it's done.

  2. We should print out something when the export has completed successfully.

@jaidhyani jaidhyani mentioned this pull request Feb 21, 2024
Copy link
Collaborator

@joshuawe joshuawe left a comment

Choose a reason for hiding this comment

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

This branch is behind main branch, so a rebase is necessary

setup.py Show resolved Hide resolved
@menamerai
Copy link
Collaborator Author

@joshuawe I rebased, not sure if I did that correctly. Does it look ok?

@menamerai menamerai force-pushed the 36-static-assets-in-python-package branch from 0ee3075 to b45d63f Compare February 21, 2024 17:01
@menamerai menamerai force-pushed the 36-static-assets-in-python-package branch from b45d63f to df48f25 Compare February 21, 2024 17:37
@menamerai menamerai merged commit 4409a9e into main Feb 23, 2024
1 check passed
@menamerai menamerai deleted the 36-static-assets-in-python-package branch February 23, 2024 17:10
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.

static assets in python package
3 participants