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

Show proper error to user when conf.py is not found #3326

Merged
merged 6 commits into from Dec 21, 2017

Conversation

@humitos
Copy link
Member

@humitos humitos commented Nov 27, 2017

I'm using BuildEnvironmentError for now, but we should use
ProjectConfigurationError once the PR got merged: #3310

Closes #2963

@agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Dec 1, 2017

I'll wait until #3310 is merged and this is rebased on there for review. Blocking for now -- or feel free to rebase there and we'll swap the PR base after merge of #3310.

@humitos humitos force-pushed the humitos/confpy/better-error branch from c488a9f to 6ba42b3 Dec 12, 2017
@humitos humitos requested review from agjohnson and ericholscher Dec 12, 2017
@humitos
Copy link
Member Author

@humitos humitos commented Dec 12, 2017

I added a couple of tests for old behaviour and also to catch this PR behaviour. Besides, I added the exception message as a class attribute.

Please, re-review this PR when you have some time.

(the last commit is styling, so you can skip it from the logic review)

@humitos
Copy link
Member Author

@humitos humitos commented Dec 12, 2017

"There are more than one conf.py file and none of them say doc "
"in their path, we don't know which one use. Please, select "
"the correct one under the Advanced settings tab in the "
"project's Admin."
Copy link
Member

@ericholscher ericholscher Dec 12, 2017

We should probably provide a link here.

Copy link
Member Author

@humitos humitos Dec 12, 2017

Do we support HTML content on the message exception? I think no.

For example, we are not using | safe filter at https://github.com/rtfd/readthedocs.org/blob/bb7bb4d6751ecce67ba274a32100406b951f9b90/readthedocs/templates/builds/build_detail.html#L156

Copy link
Member

@ericholscher ericholscher Dec 12, 2017

Would make for a better UX, perhaps as an idea in the future. We could also add it as a "build tip"

Copy link
Member Author

@humitos humitos Dec 12, 2017

Following with your idea, I'd say that the error is OK but this could be mixed with what you have done for setup.py install and other cases.

I think we should create a "Build tip" concept that checks for this type of errors and show a user friendly-shiny message with links and more.

So, in this particular case, for example, I'd rewrite the exception message to something like:

There are more than one conf.py file and we don't know which one use.

Then, a "Build tip" saying:

Please, select the correct one under the Advanced Settings (link) tab in the project's Admin or re-structure your project to have a docs folder with the conf.py inside it.

Copy link
Contributor

@agjohnson agjohnson Dec 13, 2017

I like that idea. I think we need to firm up our usage around build tips and not do the logic in the template though.

raise ProjectConfigurationError(
ProjectConfigurationError.MULTIPLE_CONF_FILES
)

Copy link
Member

@ericholscher ericholscher Dec 12, 2017

I feel like we keep changing build logic that breaks people's build. Is there a reason we are wanting to fail builds on this, instead of warning users? I think we probably want to warn them in this case, because changing this logic could easily break hundreds of project's builds for no real reason.

We did this with the setup.py install failures, and I don't think it's a good pattern to follow.

Copy link
Member Author

@humitos humitos Dec 12, 2017

I didn't change the logic here. I just added a better message.

Before this PR, it said "Conf.py not found" but there were more than one conf.py file. Now, it will fail as it was failing before but saying "Hey, too many conf.py files. We don't know which one to pick"

Copy link
Member

@ericholscher ericholscher Dec 12, 2017

hmm, interesting.

Copy link
Contributor

@agjohnson agjohnson left a comment

Looks great! 👍 on more tests here.

I noted a copy change, but this looks good otherwise. Let's keep thinking about how to best surface build suggestions, as I think this is a good pattern so far. I'll open a separate ticket to start discussing how to better implement the build changes, as I think too much logic is being hidden in templates currently.

'There are more than one conf.py file and none of them say doc '
"in their path, we don't know which one use. Please, select "
'the correct one under the Advanced settings tab in the '
"project's Admin."
Copy link
Contributor

@agjohnson agjohnson Dec 13, 2017

A few notes on copy here:

  • More clarity around why this is a problem is good -- that is: "We found more than one conf.py and are not sure which one to use." instead of mentioning docs/
  • "Please, select the correct one" -> "Please specify the correct file"

"There are more than one conf.py file and none of them say doc "
"in their path, we don't know which one use. Please, select "
"the correct one under the Advanced settings tab in the "
"project's Admin."
Copy link
Contributor

@agjohnson agjohnson Dec 13, 2017

I like that idea. I think we need to firm up our usage around build tips and not do the logic in the template though.

@humitos humitos force-pushed the humitos/confpy/better-error branch from ee1bbde to 30b908e Dec 15, 2017
@humitos
Copy link
Member Author

@humitos humitos commented Dec 15, 2017

This PR should be ready to merge.

The linting error is unrelated.

File "/home/travis/build/rtfd/readthedocs.org/.tox/lint/lib/python2.7/site-packages/pylint_django/plugin.py", line 22, in register

    start = name_checker.config.const_rgx.pattern[:-2]

AttributeError: 'NoneType' object has no attribute 'pattern'

I didn't find why it's failing yet.

@@ -171,8 +172,8 @@ class Project(models.Model):

# Other model data.
path = models.CharField(_('Path'), max_length=255, editable=False,
Copy link
Member

@stsewd stsewd Dec 15, 2017

In some parts the function call is in this way

func(arg1, 
        arg2)

and other parts like this

func(
    arg1,
    arg2)

Which style is the correct?

Copy link
Member Author

@humitos humitos Dec 15, 2017

The second one, but with a trailing comma and the ´)´ in the folowing line is closer to what we want. Like this

func(
    arg1,
    arg2,
)

but we use this only when it doesn't fit in 80 columns.

We have a style guide for this: http://docs.readthedocs.io/en/latest/contribute.html?highlight=contributing#contributing-to-development

We are using pre-commit command that runs a couple of scripts to apply the proper style to the code.

@humitos
Copy link
Member Author

@humitos humitos commented Dec 18, 2017

The linting error on this PR is solved in #3408

@humitos humitos force-pushed the humitos/confpy/better-error branch from 30b908e to 21d63bb Dec 20, 2017
@humitos
Copy link
Member Author

@humitos humitos commented Dec 21, 2017

I think this PR is read to merge. Can you review it again, please?

@ericholscher ericholscher merged commit e0c5658 into master Dec 21, 2017
1 check passed
@stsewd stsewd deleted the humitos/confpy/better-error branch Aug 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants