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 configuration file handling and move stat method and version there #47

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

valeriupredoi
Copy link
Collaborator

@valeriupredoi valeriupredoi commented Nov 16, 2022

Closes #34

Description

  • I added two yaml configuration files, one for Posix, another for S3;
  • loading of each file is done via a kwarg storage_type passed to Active class
  • these files are very basic so far: just "version" and "methods" keys so far; the methods actually work fine for Posix ->
  • the method is picked up from the methods key in the config and converted to an actual Python method
  • version works fine too

@codecov
Copy link

codecov bot commented Nov 16, 2022

Codecov Report

Base: 86.78% // Head: 87.70% // Increases project coverage by +0.92% 🎉

Coverage data is based on head (8ae7999) compared to base (e933f5b).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #47      +/-   ##
==========================================
+ Coverage   86.78%   87.70%   +0.92%     
==========================================
  Files           6        6              
  Lines         401      423      +22     
==========================================
+ Hits          348      371      +23     
+ Misses         53       52       -1     
Impacted Files Coverage Δ
activestorage/active.py 96.73% <100.00%> (+1.31%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Comment on lines +21 to +23
config_file = base_path / Path("config-s3-storage.yml")
elif storage_type == "Posix":
config_file = base_path / Path("config-Posix-storage.yml")

Choose a reason for hiding this comment

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

For "production" it would be better to pull in config from a location outside of the Python installation.

@@ -0,0 +1,6 @@
version: 1
methods:
min: np.min

Choose a reason for hiding this comment

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

Perhaps the defaults should live in Python, with YAML providing overrides only?

@markgoddard
Copy link

I had a thought about this - as a library (rather than an application), should PyActiveStorage consume a configuration file? Or would it make more sense to pass this information in via Python parameters? For the S3 case, we might consider again a Storage object interface, provided by the application to the library that knows how to connect to S3. Alternatively, each file/S3 object might be represented as a Python object that "knows" how to access it and perform reductions.

We might need some basic configuration for the test cases, which act as an application.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request standards testing testing duh
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Introduce the controlled vocabulary for operations into PyActiveStorage
2 participants