Skip to content

Change a few configuration file options from required to not required #10303

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

Merged
merged 1 commit into from
May 12, 2023

Conversation

agjohnson
Copy link
Contributor

@agjohnson agjohnson commented May 7, 2023

These are not required in our configuration specification

Resolves #10302


📚 Documentation previews 📚

These are not required in our configuration specification

Resolves #10302
@agjohnson agjohnson requested a review from a team as a code owner May 7, 2023 23:11
@agjohnson agjohnson requested a review from benjaoming May 7, 2023 23:11
Copy link
Contributor

@benjaoming benjaoming left a comment

Choose a reason for hiding this comment

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

LGTM 👍

This is a very good case for having more automatic apidocs-like generation of the configuration file schema. But we'd probably have to rework a lot of our schema parsing for that to happen :)

Btw some more changes are in the pipeline in #9921

Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

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

I think the way the documentation is written makes this to be tricky.

For example, conda is not required at all. However, if you use the conda key, then conda. enviroment becomes required. I think this is what the documentation tries to communicate, but it's not clear.

@agjohnson
Copy link
Contributor Author

@humitos aye, this is what I figured as well. I think this PR is at least more correct up front, but do agree we need a different way of describing when fields conditionally become required.

I could make this instead Required: when using conda key or something similar perhaps? Any preference?

@humitos
Copy link
Member

humitos commented May 11, 2023

I don't have any preference currently. I think we need to come back to this page anyways for a bigger refactor since it's one of the most linked page and we recommend it a lot. So, I think we should do some small research and se how other projects are presenting a config file reference and probably copy the one we like the most.

This page has really good information on it, but it just needs to be re-structured in a more clearer way only.

@benjaoming
Copy link
Contributor

I could make this instead Required: when using conda key or something similar perhaps? Any preference?

I think that sounds good to get this PR in and maintain correctness 👍

It would be super nice to do a refactor of this page! What I currently like about it is that everything has an anchor link so you can reference all options directly - and that everything is in the same page.

What I don't like is that the schema's nested structures are unclear and it lacks more copy-paste friendly examples.

@@ -297,7 +297,7 @@ conda.environment
The path to the Conda `environment file <https://conda.io/projects/conda/en/latest/user-guide/tasks/manage-environments.html>`_, relative to the root of the project.

:Type: ``path``
:Required: ``true``
:Required: ``false``
Copy link
Contributor

@benjaoming benjaoming May 12, 2023

Choose a reason for hiding this comment

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

Suggested change
:Required: ``false``
:Required: when using the ``conda`` key

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll leave this change for later for now, as we have a number of fields we should do this for.

@agjohnson agjohnson merged commit a7926d1 into main May 12, 2023
@agjohnson agjohnson deleted the agj/config-required branch May 12, 2023 18:25
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.

Docs: configuration file schema required fields are wrong
3 participants