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 agjohnson added the Status: blocked Issue is blocked on another issue label Dec 1, 2017
@agjohnson
Copy link
Contributor

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 removed the Status: blocked Issue is blocked on another issue label Dec 12, 2017
@humitos
Copy link
Member Author

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 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

Choose a reason for hiding this comment

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

We should probably provide a link here.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

hmm, interesting.

Copy link
Contributor

@agjohnson agjohnson left a comment

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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
Copy link
Member Author

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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 commented Dec 18, 2017

The linting error on this PR is solved in #3408

@humitos
Copy link
Member Author

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
@stsewd stsewd deleted the humitos/confpy/better-error branch August 15, 2018 22:15
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.

5 participants