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

Document the dependencies graph code #173

Merged
merged 5 commits into from
Nov 13, 2021

Conversation

autoantwort
Copy link
Contributor

@autoantwort autoantwort commented Aug 25, 2021

The dependency code is imo hard to understand for new developers. 2000 lines of code without a comment. Nowhere general concepts are explained. Then there is a mixture of classic mode and manifest mode code. That nearly all functions of the class VersionedPackageGraph have the name add_constraint also does not really help 😅 (without a debugger really hard to understand).

I renamed some functions to make it easier to understand the code. The whole thing startet because I wanted to implement "default-features": false as default for host dependencies.

One of the questions currently: What is the purpose of the FeatureSpec (PackageSpec + one feature)? It seems that it is used to create the installation plan

@autoantwort autoantwort force-pushed the try-adding-documentation branch 2 times, most recently from 3161191 to 0e8fa97 Compare August 28, 2021 22:57
@autoantwort autoantwort changed the title WIP: Try to document the code [Help welcomed] Try to document the dependencies graph code [Help welcomed] Aug 29, 2021
Copy link
Contributor

@strega-nil-ms strega-nil-ms left a comment

Choose a reason for hiding this comment

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

@ras0219-msft needs to review this.

@autoantwort autoantwort force-pushed the try-adding-documentation branch 2 times, most recently from 81abdbe to 0f19e33 Compare September 25, 2021 14:14
@autoantwort
Copy link
Contributor Author

@strega-nil-ms @ras0219-msft Thank you for your help, I think I now understood how the algorithm worked :)

@autoantwort autoantwort force-pushed the try-adding-documentation branch from 0f19e33 to 7cd9c29 Compare October 5, 2021 13:40
autoantwort and others added 3 commits October 6, 2021 12:32
The function enable_default_features was only called when "core" was not in dep.features, but the function only had an effect when default_features was false, but it is only false when "core" is in dep.features. So the function does nothing at all.
Co-authored-by: nicole mazzuca <[email protected]>
Co-authored-by: Robert Schumacher <[email protected]>
@autoantwort autoantwort force-pushed the try-adding-documentation branch from 7cd9c29 to 3355a22 Compare October 6, 2021 10:32
@autoantwort
Copy link
Contributor Author

You can merge this now :)

@autoantwort autoantwort changed the title Try to document the dependencies graph code [Help welcomed] Document the dependencies graph code Oct 6, 2021
@autoantwort
Copy link
Contributor Author

Any news?

@ras0219-msft
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ras0219-msft ras0219-msft merged commit bb0d337 into microsoft:main Nov 13, 2021
@ras0219-msft
Copy link
Contributor

This LGTM, thanks for the code quality improvements!

@autoantwort autoantwort deleted the try-adding-documentation branch February 5, 2022 18:43
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.

3 participants