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

Docs: add documentation for GroupPath #3952

Closed

Conversation

chrisjsewell
Copy link
Member

closes #3913

@chrisjsewell chrisjsewell requested a review from sphuber April 21, 2020 11:57
@codecov
Copy link

codecov bot commented Apr 21, 2020

Codecov Report

Merging #3952 into develop will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #3952      +/-   ##
===========================================
+ Coverage    78.45%   78.45%   +0.01%     
===========================================
  Files          461      461              
  Lines        34077    34077              
===========================================
+ Hits         26731    26732       +1     
+ Misses        7346     7345       -1     
Flag Coverage Δ
#django 70.50% <ø> (+0.01%) ⬆️
#sqlalchemy 71.35% <ø> (ø)
Impacted Files Coverage Δ
aiida/transports/plugins/local.py 80.47% <0.00%> (+0.26%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f7ac506...3cc9a03. Read the comment docs.

Copy link
Member

@ltalirz ltalirz left a comment

Choose a reason for hiding this comment

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

Thanks a lot @chrisjsewell , this looks very useful!

My main question here would be whether this is understandable to a newcomer, and so may I suggest that instead of @sphuber perhaps @mbercx gives this a review?

docs/source/working_with_aiida/groups.rst Outdated Show resolved Hide resolved
@chrisjsewell chrisjsewell requested a review from mbercx April 24, 2020 15:24
mbercx
mbercx previously approved these changes Apr 27, 2020
Copy link
Member

@mbercx mbercx left a comment

Choose a reason for hiding this comment

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

Very clear, thanks Chris! The examples really help for understanding the concepts. I wasn't aware of this feature, and now I'll definitely be using it going forward. :)

One part that was a little confusing is how you can create and delete Groups via the GroupPath. It wasn't immediately clear up to that point that you can construct GroupPaths that don't exist, but I suppose that's why they are virtual.

Another small note: Are you sure the output of Out[10] is correct? It should probably be False, considering the next line.

Copy link
Contributor

@sphuber sphuber left a comment

Choose a reason for hiding this comment

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

Thanks @chrisjsewell , since @mbercx already reviewed on the intelligibility, I won't comment on that. Seems fine. Just to reiterate his question whether the one statement is correct. Besides that, two more requests:

  • Could you please add a sentence or two on the function of cls in the GroupPath and the current limitations?
  • Please maintain one sentence per line for text.

docs/source/working_with_aiida/groups.rst Outdated Show resolved Hide resolved
@chrisjsewell
Copy link
Member Author

Please maintain one sentence per line for text.

Do you mean converting this:

You can also traverse directly through the nodes of a path,
optionally filtering by node class and any other filters allowed by the `QueryBuilder`::

to this:

You can also traverse directly through the nodes of a path, optionally filtering by node class and any other filters allowed by the `QueryBuilder`::

because you end up with quite long lines, which is why I'd added a few breaks on commas.

@chrisjsewell
Copy link
Member Author

Could you please add a sentence or two on the function of cls in the GroupPath and the current limitations?

Added in 506a6c6

@CasperWA
Copy link
Contributor

Should this be added before revamping the docs?

@chrisjsewell
Copy link
Member Author

I think so. Its ready for merging IMO

@CasperWA CasperWA requested review from mbercx, sphuber and ltalirz April 30, 2020 15:16
@sphuber
Copy link
Contributor

sphuber commented Apr 30, 2020

because you end up with quite long lines, which is why I'd added a few breaks on commas.

Yes, that is what I mean, because that is the style rule that was decided. We should not add arbitrary line breaks but just let the editor use soft line-wrapping.

Regarding the merging. Since anyway this will have to be moved once we have the new skeleton, there is no rush right? Since it is purely adding new text, there won't be any merge conflicts. So I actually would hold off with merging this until the skeleton is inplace, and then we can add it in the correct place straight away. Doing the same with my open docs PR #3966

@chrisjsewell
Copy link
Member Author

Please maintain one sentence per line for text.

Fixed in 3cc9a03

@CasperWA
Copy link
Contributor

Regarding the merging. Since anyway this will have to be moved once we have the new skeleton, there is no rush right? Since it is purely adding new text, there won't be any merge conflicts. So I actually would hold off with merging this until the skeleton is inplace, and then we can add it in the correct place straight away. Doing the same with my open docs PR #3966

Fine. Then I'll add it to the project instead.

@CasperWA CasperWA added the pr/on-hold PR should not be merged label Apr 30, 2020
@ltalirz ltalirz removed their request for review April 30, 2020 20:54
@csadorf csadorf self-requested a review May 6, 2020 14:48
@sphuber sphuber changed the title Add documentation for GroupPath Docs: add documentation for GroupPath May 11, 2020
@csadorf csadorf linked an issue May 25, 2020 that may be closed by this pull request
@csadorf
Copy link
Contributor

csadorf commented May 28, 2020

@chrisjsewell Could you adapt this PR so that we can merge it into docs-revamp?

@chrisjsewell
Copy link
Member Author

Could you adapt this PR so that we can merge it into docs-revamp?

On it now 👍

@chrisjsewell
Copy link
Member Author

rebased to #4126

@chrisjsewell chrisjsewell deleted the fix-3913-grouppath-docs branch January 27, 2021 04:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/on-hold PR should not be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Document New Group Utilities
6 participants