Skip to content

Clean up the build process#38

Merged
ctrueden merged 16 commits intomasterfrom
build-cleanup
Jun 7, 2022
Merged

Clean up the build process#38
ctrueden merged 16 commits intomasterfrom
build-cleanup

Conversation

@gselzer
Copy link
Copy Markdown
Member

@gselzer gselzer commented Apr 21, 2022

This PR is designed to incorporate the lessons learned from improving the build process of pyimagej and imglyb.

@gselzer gselzer force-pushed the build-cleanup branch 5 times, most recently from 52856dd to 105df42 Compare April 21, 2022 15:57
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Apr 21, 2022

Codecov Report

Merging #38 (3dbb6a4) into master (b63a664) will increase coverage by 1.44%.
The diff coverage is 93.19%.

@@            Coverage Diff             @@
##           master      #38      +/-   ##
==========================================
+ Coverage   77.76%   79.20%   +1.44%     
==========================================
  Files           5        6       +1     
  Lines         697      755      +58     
==========================================
+ Hits          542      598      +56     
- Misses        155      157       +2     
Impacted Files Coverage Δ
src/scyjava/config/__init__.py 57.33% <0.00%> (ø)
src/scyjava/__init__.py 70.84% <84.90%> (+1.42%) ⬆️
tests/test_version.py 87.09% <87.09%> (ø)
tests/test_convert.py 100.00% <100.00%> (+3.24%) ⬆️
tests/test_jvm.py 100.00% <100.00%> (+5.88%) ⬆️
tests/test_pandas.py 100.00% <100.00%> (+2.27%) ⬆️

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 b63a664...3dbb6a4. Read the comment docs.

@gselzer gselzer force-pushed the build-cleanup branch 2 times, most recently from 33dc072 to d079163 Compare April 21, 2022 18:01
@gselzer gselzer marked this pull request as ready for review April 21, 2022 18:26
@gselzer gselzer force-pushed the build-cleanup branch 2 times, most recently from 709726b to 121e502 Compare April 21, 2022 18:30
@gselzer
Copy link
Copy Markdown
Member Author

gselzer commented Apr 21, 2022

Major changes:

  • Bumped minimum Python version to 3.7
  • Rename environment-test.yml to dev-environment.yml and python-test-conda.yml to build.yml.
  • Track version using setuptools_scm
  • Added scyjava.__version__, and scyjava.version.version. The latter is generated during the build process, and the former will differ based on the repo status 😦
  • Added the @module_property decorator; these are lru_caches by default.
  • Added a flaking action using flake8, aligning our project with PEP 8

Open questions:

  • The return of scijava.__version__ differs based on the state of your code. scijava.version.version only exists if you have a distribution, in which case scijava.__version__ will delegate to that. If you do not have a distribution, then scijava.__version__ uses another method (either importlib.metadata.version, if your Python is >=3.8, or pkg_resources if that is installed), but these returns differ from the return of scijava.version.version; they are less specific. If somehow none of those ways exist in the environment to get the version, then scijava.__version__ whines about not being able to determine the version. Can we do better? Do we need to do better?
  • Can we write a test to test the version?

@gselzer gselzer force-pushed the build-cleanup branch 5 times, most recently from 0f5dd5e to 3208bac Compare April 27, 2022 21:06
@gselzer
Copy link
Copy Markdown
Member Author

gselzer commented Apr 27, 2022

TODO: clean up @module_property - @ctrueden suggested it should be renamed @constant, which I like.

We also have to make sure that these things cannot be mutated.

@gselzer gselzer requested a review from ctrueden April 28, 2022 16:32
@gselzer gselzer force-pushed the build-cleanup branch 3 times, most recently from 607e79e to 5d9a2c4 Compare April 28, 2022 16:47
@gselzer
Copy link
Copy Markdown
Member Author

gselzer commented Apr 28, 2022

I think this is ready for review @ctrueden.

I added a flaking job to Github Actions; this enforces code style. You can check the code style locally using flake8, which was added to the development environments. If you have flaking errors, they'll likely be fixed using autopep8, also added to the development environments. All of the flaking infrastructure is added in the last commit, so if you have a good reason against this addition we can easily remove it.

gselzer added 11 commits June 7, 2022 12:26
This makes it easier to read
We were already depending on it for code coverage, so let's just make
the full switch
We instead rely on setuptools_scm to manage the files added to the
sdist.

The discussion on whether test and documentation files belong in the
sdist is an ongoing one (see
    https://discuss.python.org/t/should-sdists-include-docs-and-tests/14578?page=5
), and it's my impression that the participants in the dicussion want to
leave this up to the tool building the sdist. So we will do that!
Since we cache by default, this decorator was intended to establish
constants.

Of course, in the wild west that is Python, someone could just
"overwrite" our constants, but it helps to declare our intentions with
the functions being decorated
Function calls as default arguments are really misleading. Default
arguments are evaulated at function definition time, NOT when the
function is called. While in this particular case, we are accessing a
global variable, leading to a case where no harm is done, we should
avoid this generally speaking.
This is a common convention in Python to indicate that variables are
deliberately discarded. See https://stackoverflow.com/a/5893946
@ctrueden ctrueden merged commit af77863 into master Jun 7, 2022
@ctrueden ctrueden deleted the build-cleanup branch June 7, 2022 17:32
@ctrueden
Copy link
Copy Markdown
Member

ctrueden commented Jun 7, 2022

Yay! Thanks, @gselzer!

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.

3 participants