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

Update function declarations to make docs reproducible #579

Merged
merged 1 commit into from Jan 21, 2019

Conversation

Projects
None yet
4 participants
@lamby
Copy link
Contributor

lamby commented Jan 17, 2019

Whilst working on the Reproducible Builds effort, we noticed that satpy's documentation could not be built reproducibly. This is because it uses the absolute buildpath in the generated docs.

This was first reported in Debian as bug #919566.

Please make the documentation reproducible
Whilst working on the Reproducible Builds effort [0], we noticed
that satpy's documentation could not be built reproducibly.

This is because it uses the absolute buildpath in the generated
docs.

This was first reported in Debian as bug #919566.

 [0] https://reproducible-builds.org/
 [0] https://bugs.debian.org/919566
@mraspaud

This comment has been minimized.

Copy link
Member

mraspaud commented Jan 17, 2019

Hi, thanks for your contribution.
IMHO, this reduces readability though, would you mind justifying your approach please ?

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jan 17, 2019

Coverage Status

Coverage decreased (-0.009%) to 77.424% when pulling 92a9271 on lamby:reproducible-build into 417b29f on pytroll:master.

1 similar comment
@coveralls

This comment has been minimized.

Copy link

coveralls commented Jan 17, 2019

Coverage Status

Coverage decreased (-0.009%) to 77.424% when pulling 92a9271 on lamby:reproducible-build into 417b29f on pytroll:master.

@codecov

This comment has been minimized.

Copy link

codecov bot commented Jan 17, 2019

Codecov Report

Merging #579 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #579      +/-   ##
==========================================
+ Coverage   77.43%   77.44%   +0.01%     
==========================================
  Files         136      136              
  Lines       19143    19153      +10     
==========================================
+ Hits        14823    14833      +10     
  Misses       4320     4320
Impacted Files Coverage Δ
satpy/composites/__init__.py 64.16% <100%> (+0.1%) ⬆️
satpy/scene.py 84.13% <100%> (+0.06%) ⬆️
satpy/readers/__init__.py 94.46% <100%> (+0.07%) ⬆️
satpy/config.py 47.82% <100%> (+1.15%) ⬆️

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 417b29f...92a9271. Read the comment docs.

@lamby

This comment has been minimized.

Copy link
Contributor Author

lamby commented Jan 17, 2019

@mraspaud

would you mind justifying your approach please ?

Not entirely sure what you mean by this, sorry. (Or, what do you not find readable about this?)

@mraspaud

This comment has been minimized.

Copy link
Member

mraspaud commented Jan 17, 2019

Sorry I wasn't clear: I meant why does the current implementation break the reproducibility of the documentation ?

About readability, I just find that turning eg this:

def get_environ_config_dir(default=PACKAGE_CONFIG_PATH):

into this:

def get_environ_config_dir(default=None):
    if default is None:
	    default = PACKAGE_CONFIG_PATH

hurts readability and documentation a bit: default is not None, it is an actual path.

But maybe that's just me :)

@djhoese

This comment has been minimized.

Copy link
Contributor

djhoese commented Jan 17, 2019

While I agree with @mraspaud that the code becomes a little uglier, I think the benefits outweigh the ugliness. It moves the config directory determination to runtime rather than "compile time".

Additionally, this is something (similar) to what I am going to need to do in the future when I start using the donfig package as SatPy's configuration backend. This way users could easily modify the "default config" location during execution and it be retrieved from the global configuration object inside functions/classes.

I am OK merging this.

@djhoese djhoese changed the title Please make the documentation reproducible Update function declarations to make docs reproducible Jan 17, 2019

@lamby

This comment has been minimized.

Copy link
Contributor Author

lamby commented Jan 17, 2019

Sorry I wasn't clear: I meant why does the current implementation break the reproducibility of the documentation ?

Because it will encode the current build path via that constant (eg. /home/lamby/foo on my machine, /home/mraspaud/bar on yours gets embedded into the generated HTML) and we thus different results → not reproducible. :)

@mraspaud

This comment has been minimized.

Copy link
Member

mraspaud commented Jan 21, 2019

Ok, I'm merging this then

@mraspaud mraspaud merged commit 8288d4b into pytroll:master Jan 21, 2019

5 of 7 checks passed

CodeFactor 1 issue found.
Details
coverage/coveralls Coverage decreased (-0.009%) to 77.424%
Details
codecov/patch 100% of diff hit (target 77.43%)
Details
codecov/project 77.44% (+0.01%) compared to 417b29f
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
stickler-ci No lint errors found.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment