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

🔧 MAINTAIN: Improve repo configuration #112

Merged
merged 1 commit into from
Aug 30, 2021

Conversation

chrisjsewell
Copy link
Member

@chrisjsewell chrisjsewell commented Aug 28, 2021

  • Move config to setup.cfg and pyproject.toml
  • Add Manifest.in, tox.ini
  • Replace requirements.txt/dev-requirements.txt with requirements.lock
  • Split dev extra to dev & examples
  • Move from yapf to black code formatting
  • Add more pre-commit hooks
  • Update pylint version and fix new failures
  • Drop python 3.6

@codecov
Copy link

codecov bot commented Aug 28, 2021

Codecov Report

Merging #112 (34dd190) into develop (a2561a4) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 34dd190 differs from pull request most recent head a92c695. Consider uploading reports for the commit a92c695 to get more accurate results
Impacted file tree graph

@@            Coverage Diff            @@
##           develop      #112   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            7         7           
  Lines         1541      1544    +3     
=========================================
+ Hits          1541      1544    +3     
Impacted Files Coverage Δ
disk_objectstore/__init__.py 100.00% <100.00%> (ø)
disk_objectstore/container.py 100.00% <100.00%> (ø)
disk_objectstore/examples/example_objectstore.py 100.00% <100.00%> (ø)
disk_objectstore/examples/profile_zeros.py 100.00% <100.00%> (ø)
disk_objectstore/models.py 100.00% <100.00%> (ø)
disk_objectstore/utils.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a2561a4...a92c695. Read the comment docs.

- Move config to `setup.cfg` and `pyproject.toml`
- Add `Manifest.in`, `tox.ini`
- Replace `requirements.txt`/`dev-requirements.txt with `requirements.lock`
- Move from yapf to black code formatting
- Add more pre-commit hooks
- Update pylint version and fix new failures
- Drop python 3.6
Copy link
Member

@ramirezfranciscof ramirezfranciscof left a comment

Choose a reason for hiding this comment

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

Ok, this is all good for me! I just made a couple of comments and questions out of curiosity.

One general question I still have is why we are changing yapf to black in this repo. I am assuming this was either discussed on the last aiida meeting, or with @giovannipizzi more directly, and he agreed to do the change (I mean, that part I think should be greenlighted by him rather than me). In that case feel free to go ahead and merge!

Comment on lines +5 to +12
- Various general (but very important) speed improvements [\[#96\]](https://github.com/aiidateam/disk-objectstore/pull/96) [\[#102\]](https://github.com/aiidateam/disk-objectstore/pull/102)
- Add callbacks to a number of functions (e.g. export, add_objects_to_pack, ... to allow showing progress bars or similar indicators [\[#96\]](https://github.com/aiidateam/disk-objectstore/pull/96)
- Implement repacking (at least when not changing hashing or compression) [\[#96\]](https://github.com/aiidateam/disk-objectstore/pull/96)
- Remove `export` function, implement `import_objects` function instead, to be called on the other side (it's more efficient) [\[#96\]](https://github.com/aiidateam/disk-objectstore/pull/96)
- Add support for VACUUMing operations on the SQLite database (very important for efficiency) [\[#96\]](https://github.com/aiidateam/disk-objectstore/pull/96)
- Add support for multiple hashing algorithms [\[#96\]](https://github.com/aiidateam/disk-objectstore/pull/96)
- Add concept of (unique) `container_id` [\[#97\]](https://github.com/aiidateam/disk-objectstore/pull/97)
- Generalize the compression algorithm implementation, and multiple algorithms are supported now [\[#99\]](https://github.com/aiidateam/disk-objectstore/pull/99)
Copy link
Member

Choose a reason for hiding this comment

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

=/ not sure why git doesn't recognize these as just being a few lines below...strange

Copy link
Member Author

Choose a reason for hiding this comment

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

The backslash escapes in the link names [\[#97\]]

Comment on lines +140 to +147
print(f"Time to retrieve {num_files} loose objects: {tot_time:.4} s")

# Check that the content is correct
for filename in retrieved:
assert retrieved[filename] == files[filename], 'Mismatch (content) for {}, {} vs {}'.format(
filename, retrieved[filename], files[filename]
for filename, item in retrieved.items():
assert (
item == files[filename]
), "Mismatch (content) for {}, {} vs {}".format(
filename, item, files[filename]
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason for changing the above print to fstrings but not the string in the assert?

Copy link
Member Author

Choose a reason for hiding this comment

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

This was all done automatically by the pyupgrade pre-commit hook. Guess it just missed a few for whatever reason

Comment on lines 9 to 11
Creating 10000 files with UUID: 1.33 s
Creating 10000 files with tempfile: 1.78 s No newline at end of file
Creating 10000 files with tempfile: 1.78 s
Copy link
Member

Choose a reason for hiding this comment

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

Uhm, this is weird, these lines look the same...

I'm guessing black doing something strange with the end of line/file characters that are not visually shown? There is stuff like this in many other files too.

Comment on lines +37 to +49
[options.extras_require]
dev =
coverage
pre-commit
pytest
pytest-benchmark
pytest-cov
examples =
click
memory-profiler
profilehooks
psutil
pywin32;platform_system == "Windows"
Copy link
Member

Choose a reason for hiding this comment

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

Out of curiosity, why are we using setup.cfg here instead of setup.json like in our other repos?

Copy link
Member Author

Choose a reason for hiding this comment

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

setup.json is a bespoke aiida solution, not supported by any PEP. The only reason we use it, is for compatibility with the aiida-registry (which eventually we should fix: aiidateam/aiida-registry#132, but this will be easier when everything moves to pyproject.toml)

@chrisjsewell
Copy link
Member Author

One general question I still have is why we are changing yapf to black in this repo.

Oh they will all be getting changed: yapf was picked for whatever reason historically, but now black is easily the de facto standard; https://python.libhunt.com/compare-yapf-vs-black (and from a technical standpoint, I can tell you the way black parses the code is a lot more sound)

@chrisjsewell chrisjsewell merged commit 2cb2841 into aiidateam:develop Aug 30, 2021
@chrisjsewell chrisjsewell deleted the update-repo branch August 30, 2021 14:07
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