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: Fix typo's in "Internal Architecture - Repositories" section #4886

Merged
merged 4 commits into from
May 5, 2021

Conversation

mbercx
Copy link
Member

@mbercx mbercx commented Apr 29, 2021

Fixes #4036

Here we fix some typos in the "Internal Architecture - Repositories"
section, as well as restructure the page so the current repository design is
presented first. The original design is still explained at the end of the section
since it can offer some insight into the motivation behind the current design.

Copy link
Member

@ramirezfranciscof ramirezfranciscof left a comment

Choose a reason for hiding this comment

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

Keen eyes ;)

@sphuber
Copy link
Contributor

sphuber commented Apr 29, 2021

Can we use this PR to also potentially change the reordering of the sections? At least I remember now that during the reviewing of the original new repo PR @giovannipizzi suggested that we change the sections and don't start with the historical perspective, but instead just explain how the current repo system works. Then after that we can give the insight in the legacy design. I'd be ok with this if others think this is also better. Would be good to include those changes in this PR then and revise the entire section since it hasn't really been reviewed yet.

@giovannipizzi
Copy link
Member

Indeed - now moving forward to the next release, it's better to start with the "current status", and then only at the end (if/when appropriate) give a historical perspective, maybe even linked in a different page if not crucial to be read by new users. I let you decide if you can do it directly in this PR (if you can't, please open an issue so we don't forget, but great if you can)

@sphuber
Copy link
Contributor

sphuber commented Apr 29, 2021

if not crucial to be read by new users.

Note that this is the "Internal Architecture" which is not intended for new users, but developers and maybe interested advanced users.

@sphuber
Copy link
Contributor

sphuber commented May 5, 2021

If we fix the ordering in this one, we could even close #4036 with this PR which would be good

@sphuber
Copy link
Contributor

sphuber commented May 5, 2021

Thanks a lot @mbercx . This is looking good to me. If you fix the warning in the docs build:

/home/circleci/project/docs/source/internals/repository.rst:94: WARNING: undefined label: internal-architecture:repository:design:requirements (if the link has no caption the label must precede a section header)

then we can merge this.

Sorry for slipping you the additional work during this PR ;)

@mbercx
Copy link
Member Author

mbercx commented May 5, 2021

Thanks a lot @mbercx . This is looking good to me. If you fix the warning in the docs build:

/home/circleci/project/docs/source/internals/repository.rst:94: WARNING: undefined label: internal-architecture:repository:design:requirements (if the link has no caption the label must precede a section header)

I know, I saw it. 🙃 Couldn't build the docs on my macBook because I just upgraded to Big Sur and apparently I had to update Xcode, which takes a gazillion years. 🙄 Now building on my work station to make sure I don't have any more warnings.

Sorry for slipping you the additional work during this PR ;)

Haha, no worries, I'm used to your tricks. 😏

@mbercx mbercx force-pushed the mbercx-patch-1 branch from 557fd7e to 8850556 Compare May 5, 2021 17:05
@codecov
Copy link

codecov bot commented May 5, 2021

Codecov Report

Merging #4886 (90d5e80) into develop (e7b9b96) will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4886      +/-   ##
===========================================
- Coverage    80.01%   80.01%   -0.00%     
===========================================
  Files          516      516              
  Lines        36573    36573              
===========================================
- Hits         29261    29260       -1     
- Misses        7312     7313       +1     
Flag Coverage Δ
django 74.48% <ø> (-<0.01%) ⬇️
sqlalchemy 73.42% <ø> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
aiida/transports/plugins/local.py 81.54% <0.00%> (-0.25%) ⬇️

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 e7b9b96...90d5e80. Read the comment docs.

@sphuber sphuber self-requested a review May 5, 2021 17:53
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.

Appreciate the accepted scope creep 👍

@sphuber sphuber merged commit d0a053a into develop May 5, 2021
@sphuber sphuber deleted the mbercx-patch-1 branch May 5, 2021 17:54
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: Internals: Data storage (File repository)
4 participants