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

First Blog #2813

Merged
merged 3 commits into from
Jun 2, 2023
Merged

First Blog #2813

merged 3 commits into from
Jun 2, 2023

Conversation

shilpiprd
Copy link
Contributor

This folder should provide a place to put all the blog posts.

@codecov
Copy link

codecov bot commented May 18, 2023

Codecov Report

Merging #2813 (0869846) into master (89e4ab6) will increase coverage by 0.02%.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2813      +/-   ##
==========================================
+ Coverage   81.45%   81.48%   +0.02%     
==========================================
  Files         144      144              
  Lines       20054    20054              
  Branches     3192     3192              
==========================================
+ Hits        16335    16340       +5     
+ Misses       2909     2906       -3     
+ Partials      810      808       -2     

see 2 files with indirect coverage changes

@shilpiprd
Copy link
Contributor Author

@skoudoro. I'm actually having trouble writing up a rst file. Since DIPY is very correlated to FURY, i figured that the structure of the rst file would be similar, but it turns out it's not.
I tried running my .rst file with the sphinx version we have in dipy, but I got error in docscrape.py file, as an import was outdated for all python version > 3.10.
I think some of the files for sphinx are a little outdated and needs to be probably modified.
Also, can you help me with the starting structure of the rst files ?

@skoudoro
Copy link
Member

Hi @shilpiprd,

Thank you for this first blog post! I will do my first review after @lb-97 and @RafaelNH.

@skoudoro
Copy link
Member

i figured that the structure of the rst file would be similar, but it turns out it's not.

it is correlated and same syntax. it is just missing an extension (ablog) that we need to add in this repository.

@shilpiprd
Copy link
Contributor Author

Yeah I actually figured that out, but I wasn't sure if it was ok for me to change an already existing file.
Also, the file : doc/sphinxext/docscrape.py Line#88 in this file imports collections.Mappinngs, but for alll python versions > 3.10, it should be collections.abc.Mappings.
This actually gave me error on my system.
Should I change that as well ?

@skoudoro
Copy link
Member

skoudoro commented May 22, 2023

Should I change that as well ?

You can add a check if python >= 3.10 import one version otherwise, the other

@shilpiprd
Copy link
Contributor Author

Hey Serge, I think this PR's commit history doens't look like it should, so should i create a new PR which contains all the required changes ? It would look cleaner that way.

@skoudoro
Copy link
Member

skoudoro commented May 26, 2023

Hi @shilpiprd,

Use this PR as a training one.

You can squash the commit to reduce the number of commit and improve commit history.
Please try to do that here.

Do NOT create a new PR. I assure you that all you future PR will be easier to manage after learning this.

EDIT: this means (rebase and squash)

@skoudoro skoudoro requested a review from lb-97 May 26, 2023 07:52
@shilpiprd shilpiprd force-pushed the blog_branch branch 3 times, most recently from 612c7f2 to 29221e2 Compare May 26, 2023 13:47
@shilpiprd
Copy link
Contributor Author

I think I squahsed all the needed commits together, "2nd_Final_commit " should be good.

@lb-97
Copy link
Contributor

lb-97 commented May 26, 2023

@shilpiprd, I think you can squash all commits to a single commit, that should suffice.

@shilpiprd
Copy link
Contributor Author

Are you sure ? Because the structure of the rst file has been removed in 2nd commit, but it's there in the final squashed commit. If I squash them then wouldn't 2nd commit overwrite all changes (i.e remove the top structural part )? Same for 1st commit, I've changed some lines in the rst blog post.

@lb-97
Copy link
Contributor

lb-97 commented May 27, 2023

If you squash them using git rebase -i HARD~3 then there shouldn't be any overriding. All commits are squashed to one change, yet to be staged with a new commit message. @shilpiprd

@shilpiprd
Copy link
Contributor Author

yeah, I think it's done now. Thanks @lb-97 !

Copy link
Member

@skoudoro skoudoro left a comment

Choose a reason for hiding this comment

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

Hi @shilpiprd,

See below my review. After that, it should be ready to go.

@lb-97, can you look at my review also to get inspired for the future one.

Thanks to both of you

Copy link
Member

@skoudoro skoudoro left a comment

Choose a reason for hiding this comment

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

be careful, you committed doc/posts/2023/2023_05_29_Shilpi_week1.pdf accidentally.
to remove

@shilpiprd
Copy link
Contributor Author

shilpiprd commented May 30, 2023

yeah it's supposed to be an rst file right?

@lb-97
Copy link
Contributor

lb-97 commented May 30, 2023

@lb-97, can you look at my review also to get inspired for the future one.

Sure @skoudoro. Thankyou.

Copy link
Member

@skoudoro skoudoro left a comment

Choose a reason for hiding this comment

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

Hi @shilpiprd,

See below my review. Also, link does not work. Please check how to do a link in rst.

Thanks

doc/posts/2023/2023_05_29_Shipi_week1.rst Outdated Show resolved Hide resolved
doc/posts/2023/2023_05_29_Shipi_week1.rst Outdated Show resolved Hide resolved
doc/posts/2023/2023_05_29_Shipi_week1.rst Outdated Show resolved Hide resolved
doc/posts/2023/2023_05_29_Shipi_week1.rst Outdated Show resolved Hide resolved
doc/posts/2023/2023_05_29_Shipi_week1.rst Outdated Show resolved Hide resolved
doc/posts/2023/2023_05_29_Shipi_week1.rst Outdated Show resolved Hide resolved
doc/posts/2023/2023_05_29_Shipi_week1.rst Outdated Show resolved Hide resolved
@shilpiprd
Copy link
Contributor Author

I hope with this all the links would work, please let me know if any other changes are required.

@skoudoro skoudoro merged commit 40c269e into dipy:master Jun 2, 2023
27 checks passed
@skoudoro
Copy link
Member

skoudoro commented Jun 2, 2023

Thanks @shilpiprd , merging

@shilpiprd shilpiprd deleted the blog_branch branch June 5, 2023 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants