Align on venv as virtual environment convention
Update makefile test target to re-use same script used in CI to orchestrate testing
Remove refererences to tox and nose
Fix dev-requirements.txt and doc-requirements.txt to pin to versions that are non-breaking

RF: Align on venv as virtual environment convention. Update makefile …

8dac77c

…test target to re-use same script used in CI to orchestrate testing. Remove refererences to tox and nose

Copy link

pep8speaks commentedMay 7, 2022•
edited
Loading

Hello@jacobdr, Thank you for updating!

Cheers! There are no style issues detected in this Pull Request. 🍻 To test for issues locally,pip install flake8 and then runflake8 nibabel.

Comment last updated at 2022-05-07 19:05:52 UTC

Copy link

codecovbot commentedMay 7, 2022•
edited
Loading

Codecov Report

Merging#1105 (ad2cc68) intomaster (7cfaebf) willnot change coverage.
The diff coverage isn/a.

@@           Coverage Diff           @@##           master    #1105   +/-   ##=======================================  Coverage   92.29%   92.29%           =======================================  Files         100      100             Lines       12247    12247             Branches     2393     2393           =======================================  Hits        11303    11303             Misses        621      621             Partials      323      323

Continue to review full report at Codecov.

Legend -Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing data
Powered byCodecov. Last update7cfaebf...ad2cc68. Read thecomment docs.

RF: Run linting check over whole source tree, use setup.cfg to config…

ad2cc68

…ure flake8 (closer aligns to pep8speaks bot behavior)

jacobdr commented

May 7, 2022

View reviewed changes

Makefile

		$(PYTHON) -c'from nisext.testers import bdist_egg_tests; bdist_egg_tests("nibabel", doctests=False, label="not script_test")'

		sdist-venv: clean
		rm -rf dist venv

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

I wasn't sure if this target is still being used anywhere -- so I kind of hacked it to install the dev dependencies to get it to pass on my local

jacobdr commented

May 7, 2022

View reviewed changes

doc-requirements.txt

		-r requirements.txt
		sphinx<3
		numpydoc
		jinja2<3

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

I needed to pin these to get version compatibility to work. In the future might be better to use a tool like poetry so that we can generate a lockfile and not have to deal with this sort of not-fun dependency pinning

Copy link

ContributorAuthor

jacobdrMay 8, 2022

Choose a reason for hiding this comment

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

I needed this to get my local to pass but it seems like CI might not like it.... I'll try reverting to see if if this is what is breaking the pre-release CI jobs

jacobdr commented

May 7, 2022

View reviewed changes

setup.cfg

		sphinx
		nibabel/externals/*
		*/__init__.py
		# temporary -- should be removed in another PR

Copy link

ContributorAuthor

jacobdrMay 7, 2022

Choose a reason for hiding this comment

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

Need a follow-up PR to clean up the remaining issues. I added these as ignores so that local flake8 passes. These were being covered up by the configuration of pep8speaksonly looking at the diff

jacobdr commented

May 7, 2022

View reviewed changes

tools/ci/activate.sh

		sourcevirtenv/bin/activate
		elif [-evirtenv/Scripts/activate ];then
		sourcevirtenv/Scripts/activate
		if [-evenv/bin/activate ];then

Copy link

ContributorAuthor

jacobdrMay 7, 2022

Choose a reason for hiding this comment

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

This change was made so that we are consistent in local dev and remote dev with the virtualenv

This was referencedMay 7, 2022

Remove temporary flake8 ignores so the entire repo can be linted in CI#1106

Open

RF: Run flake8 over all files in the repo + fix outstanding formatting issues#1107

Closed

jacobdr commented

May 8, 2022

View reviewed changes

Makefile

		test:test-styleunittest testmanual


		ut-%: build

Copy link

ContributorAuthor

jacobdrMay 8, 2022

Choose a reason for hiding this comment

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

I removed this as I did not see it being used anywhere. I can revert this if a maintainer feels it should be brought back

Copy link

ContributorAuthor

jacobdr commentedMay 8, 2022

@effigies all the pre-release tests failed for the same reason -- a string representation in a doctest fornetcdf_file. I don't immediately see how my changes would have introduced this. Looking at this now, but any thoughts as to root cause?

Copy link

ContributorAuthor

jacobdr commentedMay 8, 2022•
edited
Loading

Trying to narrow differences... sincemmap is coming from the standard lib I suspected a change in Python versions and howrepr(mmap_obj) orstr(mmap_obj) is displayed... Its possible, though still smells to me so needs a sanity check.

As evidence, though, comparing against a prior (chose#1096 as the most recent unmerged green build I saw....)

Python version comparison