Refactor BaseEnvironment #3687

stsewd · 2018-02-27T02:23:29Z

This will close #3643
Also related to #3665

stsewd · 2018-02-27T06:15:01Z

I have removed the record dependency from BaseEnvironment and BuildCommand, but still there is code repetition, I will continue refactoring it later. For now, all tests pass :)

stsewd · 2018-02-27T06:18:50Z

readthedocs/doc_builder/environments.py

+            kwargs['warn_only'] = True
+        return super(LocalEnvironment, self).run(*cmd, **kwargs)
+
+    # record, force_success, warn_only


I see that there is some magic with the parameters on the run_command_class (change according to other parameters). So I was thinking to use different parameters: record, force_success, warn_only that are independent from each other and play nice together.

stsewd · 2018-03-02T07:07:27Z

What I done so far:

Remove record dependency from BuildCommand and BaseEnvironment
Use pre_run_command and post_run_command to extend the behavior of the run method.
Use a mixin to add the record command behavior.
Previous run accepted record=True, record_as_success=False and warn_only=False. This variables had a dependence between each other. What I propose is to use only record=True and warn_only=False.

What is missing:

Change method calls to new vars (record=True and warn_only=False).
See a way to remove build_env from pre_run_command
Maybe update/add tests.

humitos

Thanks for this PR! The code looks good. Althouhg, I think it needs more work on the logic/usage of the record, record_as_success and friends.

The builders are complex, so we will want to merge #3764 first to cover some scenarios.

Also, we will need to do some manual QA to be sure that this code is working and the builders are building the documentation properly. This code is escential for the build process, it's complex to see the errors, and it's tricky to test (we had many problems each time we touch it :) ) --so, I'd like to be more careful this time.

humitos · 2018-03-23T16:20:46Z

readthedocs/doc_builder/environments.py

        :param warn_only: don't raise an exception on command failure
-        :param record_as_success: force command ``exit_code`` to be saved as


We are lossing the documentation for these attributes here. It should be added where it fit.

humitos · 2018-03-23T16:22:11Z

readthedocs/doc_builder/environments.py

@@ -348,12 +328,7 @@ def run_command_class(
        build_cmd = cls(cmd, **kwargs)
        self.commands.append(build_cmd)
        build_cmd.run()


self.pre_run_command shuold be before this line and should receive only the build_cmd, warn_only and **kwargs

humitos · 2018-03-23T16:24:27Z

readthedocs/doc_builder/environments.py

+            'build_env': self,
+        })
+        self.record = kwargs.pop('record', True)
+        self.record_as_success = warn_only


record_as_success and warn_only are not the same.

(take a look at my previous comment where the logic to calculate these is defined)

humitos · 2018-03-23T16:25:48Z

readthedocs/doc_builder/environments.py

-        if not record:
-            warn_only = True
-
-        if record_as_success:


We do need this logic to calculate these values to decide later if we want to record or just warn or both.

Yeah, I know, that's because I want to eliminate those parameters and just use record and warn_only #3687 (comment)

stsewd · 2018-03-28T18:36:57Z

I just update this PR to the current master, but I want to wait until #3842 is fixed to continue, and also to this tests #3764.

humitos · 2018-12-03T12:14:07Z

@stsewd issues mentioned "as blocking" this PR are already closed/merged.

Do you want to continue with this PR? I think it was close enough to be merged.

codecov · 2018-12-06T19:44:50Z

Codecov Report

Merging #3687 into master will decrease coverage by 2.06%.
The diff coverage is 70%.

@@            Coverage Diff             @@
##           master    #3687      +/-   ##
==========================================
- Coverage    76.8%   74.74%   -2.07%     
==========================================
  Files         158      158              
  Lines        9937     9933       -4     
  Branches     1241     1239       -2     
==========================================
- Hits         7632     7424     -208     
- Misses       1973     2177     +204     
  Partials      332      332

Impacted Files	Coverage Δ
readthedocs/doc_builder/environments.py	`80.38% <70%> (-6.69%)`	⬇️
readthedocs/vcs_support/backends/git.py	`46.2% <0%> (-36.08%)`	⬇️
readthedocs/core/symlink.py	`78.65% <0%> (-15.86%)`	⬇️
readthedocs/builds/forms.py	`83.33% <0%> (-12.5%)`	⬇️
readthedocs/doc_builder/backends/sphinx.py	`57.42% <0%> (-12.38%)`	⬇️
readthedocs/vcs_support/backends/hg.py	`53.22% <0%> (-11.3%)`	⬇️
readthedocs/vcs_support/base.py	`83.01% <0%> (-7.55%)`	⬇️
readthedocs/projects/tasks.py	`64.2% <0%> (-7%)`	⬇️
readthedocs/doc_builder/backends/mkdocs.py	`83.16% <0%> (-6.94%)`	⬇️
readthedocs/projects/validators.py	`75.38% <0%> (-3.08%)`	⬇️
... and 6 more

codecov · 2018-12-06T19:44:51Z

Codecov Report

Merging #3687 into master will decrease coverage by 2.06%.
The diff coverage is 70%.

@@            Coverage Diff             @@
##           master    #3687      +/-   ##
==========================================
- Coverage    76.8%   74.74%   -2.07%     
==========================================
  Files         158      158              
  Lines        9937     9933       -4     
  Branches     1241     1239       -2     
==========================================
- Hits         7632     7424     -208     
- Misses       1973     2177     +204     
  Partials      332      332

Impacted Files	Coverage Δ
readthedocs/doc_builder/environments.py	`80.38% <70%> (-6.69%)`	⬇️
readthedocs/vcs_support/backends/git.py	`46.2% <0%> (-36.08%)`	⬇️
readthedocs/core/symlink.py	`78.65% <0%> (-15.86%)`	⬇️
readthedocs/builds/forms.py	`83.33% <0%> (-12.5%)`	⬇️
readthedocs/doc_builder/backends/sphinx.py	`57.42% <0%> (-12.38%)`	⬇️
readthedocs/vcs_support/backends/hg.py	`53.22% <0%> (-11.3%)`	⬇️
readthedocs/vcs_support/base.py	`83.01% <0%> (-7.55%)`	⬇️
readthedocs/projects/tasks.py	`64.2% <0%> (-7%)`	⬇️
readthedocs/doc_builder/backends/mkdocs.py	`83.16% <0%> (-6.94%)`	⬇️
readthedocs/projects/validators.py	`75.38% <0%> (-3.08%)`	⬇️
... and 6 more

stsewd · 2018-12-06T21:55:55Z

@humitos turns out I forgot everything about the current state of this PR 😢 but I have read all the related code again, and there is a part blocking this. We were relying on the record param to change the value of warn_only and friends, but now BaseEnvironment shouldn't have any idea about record, so we need to remove that magical dependency and not change any other parameters based on record. I was doing that here but looks like it's big, so I'm going to do another PR.

stsewd · 2018-12-07T00:51:24Z

Ok, I think I got it, after reading #3520, but I'm not sure. We are lying to the users when saving the result to something else. But I understand that would make the build to appear like a failure... But, if the command failed, then something isn't right. Also, we are just using that to execute some commands in the VCS, actually we don't use record_as_success on git anymore.

stsewd · 2018-12-07T02:21:16Z

I was checking, we still need that for svn and bzr :/, I guess I'll continue here

stale · 2019-01-21T04:55:35Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stsewd added 5 commits February 26, 2018 21:21

Remove call to custom logging

376b825

Define interface for pre and post run of the command

8cb9561

Remove record_as_success from BuildCommand

a969798

Override run commands on BuildEnvironment

9f30295

Override run commands on LocalEnvironment

c30b916

stsewd commented Feb 27, 2018

View reviewed changes

Linter

23290a8

stsewd mentioned this pull request Feb 27, 2018

Refactor to replace old logging to avoid mangling #3677

Merged

stsewd force-pushed the refactor-base-environment branch from 5cbd078 to f6aaab3 Compare March 2, 2018 06:05

Refactor to mixin

443b0e0

stsewd force-pushed the refactor-base-environment branch from f6aaab3 to 443b0e0 Compare March 2, 2018 06:08

stsewd added 4 commits March 2, 2018 01:17

Send self

b49e896

Use only record and warn_only

754026d

Fix number of paramters

e462b48

Linter

418e74b

RichardLitt added the PR: work in progress Pull request is not ready for full review label Mar 2, 2018

stsewd mentioned this pull request Mar 2, 2018

update_repos commands fails #3696

Closed

stsewd mentioned this pull request Mar 11, 2018

[RDY] Fix tests for environment #3764

Merged

humitos requested changes Mar 23, 2018

View reviewed changes

Merge branch 'master' into refactor-base-environment

a513332

humitos added this to the Refactoring milestone Dec 3, 2018

stsewd added 3 commits December 5, 2018 16:49

Merge branch 'master' into refactor-base-environment

29a71f1

Merge branch 'master' into refactor-base-environment

e9868ac

Put back build_env

5d99d71

stsewd mentioned this pull request Dec 20, 2018

Show current executing command in build report #5019

Closed

stale bot added the Status: stale Issue will be considered inactive soon label Jan 21, 2019

stale bot closed this Jan 28, 2019

stsewd deleted the refactor-base-environment branch April 11, 2019 13:54

		:param warn_only: don't raise an exception on command failure
		:param record_as_success: force command ``exit_code`` to be saved as

Uh oh!

Refactor BaseEnvironment #3687

Refactor BaseEnvironment #3687

Uh oh!

Conversation

stsewd commented Feb 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stsewd commented Feb 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stsewd Feb 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stsewd commented Mar 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

humitos left a comment

Choose a reason for hiding this comment

Uh oh!

humitos Mar 23, 2018

Choose a reason for hiding this comment

Uh oh!

humitos Mar 23, 2018

Choose a reason for hiding this comment

Uh oh!

humitos Mar 23, 2018

Choose a reason for hiding this comment

Uh oh!

humitos Mar 23, 2018

Choose a reason for hiding this comment

Uh oh!

stsewd Mar 28, 2018

Choose a reason for hiding this comment

Uh oh!

stsewd commented Mar 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

humitos commented Dec 3, 2018

Uh oh!

codecov bot commented Dec 6, 2018

Codecov Report

Uh oh!

codecov bot commented Dec 6, 2018

Codecov Report

Uh oh!

stsewd commented Dec 6, 2018

Uh oh!

stsewd commented Dec 7, 2018

Uh oh!

stsewd commented Dec 7, 2018

Uh oh!

stale bot commented Jan 21, 2019

Uh oh!

Uh oh!

stsewd commented Feb 27, 2018 •

edited

Loading

stsewd commented Feb 27, 2018 •

edited

Loading

stsewd Feb 27, 2018 •

edited

Loading

stsewd commented Mar 2, 2018 •

edited

Loading

stsewd commented Mar 28, 2018 •

edited

Loading