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

Add reference to ReviewNB to Code Review section. #247

Merged
merged 3 commits into from
Dec 22, 2021
Merged

Conversation

svenvanderburg
Copy link
Member

Add reference to reviewing jupypter notebooks with ReviewNB to Code Review section.

Comment on lines 302 to 305
You can use [ReviewNB](https://www.reviewnb.com/) for that. If you configure
your esciencecenter.nl email address as your primary email for your Github
account [here](https://github.com/settings/emails) you have access to the
academic plan with full benefits.
Copy link
Member Author

@svenvanderburg svenvanderburg Jun 29, 2021

Choose a reason for hiding this comment

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

I am not sure if we should add the 2nd part about the academic plan. It is nice to know for escience center staff, but it is ofcourse not a generally applicable best practice.

Copy link
Member

Choose a reason for hiding this comment

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

The NLeSC guide should be targeted at NLeSC staff. All generic stuff should be in The Turing Way instead. So it's okay to suggest NLeSC specific things.

Copy link
Member Author

Choose a reason for hiding this comment

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

Check!

Copy link
Member

@c-martinez c-martinez left a comment

Choose a reason for hiding this comment

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

Couple of minor suggestions that I think would make the section more readable as a new comer.

svenvanderburg and others added 2 commits December 22, 2021 14:27
Co-authored-by: Carlos Martinez <c.martinez@esciencecenter.nl>
Co-authored-by: Carlos Martinez <c.martinez@esciencecenter.nl>
@svenvanderburg
Copy link
Member Author

@jmaassen or @c-martinez. Sorry I lost track of this... This can be merged now!

@c-martinez c-martinez self-requested a review December 22, 2021 16:32
@c-martinez c-martinez merged commit f166ce9 into master Dec 22, 2021
@c-martinez c-martinez deleted the reviewnb branch December 22, 2021 16:32
@c-martinez
Copy link
Member

Thanks @svenvanderburg !

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