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

Exemplify more Carpentries-relevant code style tools #698

Closed
wants to merge 3 commits into from
Closed

Exemplify more Carpentries-relevant code style tools #698

wants to merge 3 commits into from

Conversation

katrinleinweber
Copy link
Contributor

Since no Carpentries lesson teaches Perl, Ruby or focusses on HTML, how about mentioning tools that are relevant for our curriculum? These are suggestions which have been mentioned in a lesson, or its repository.

Related to datacarpentry/R-ecology-lesson#359, swcarpentry/shell-novice#239 & swcarpentry/r-novice-gapminder#529, and to #432.

Copy link
Contributor

@munkm munkm left a comment

Choose a reason for hiding this comment

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

I really like updating the style suggestions to what we teach in other carpentries lessons. I've made a few suggestions about the content. Let me know what you think.

Thank you for updating these! I think the learners will find them more useful in practice.

`htmltidy`, `perltidy`, `rubocop`, etc.) to enforce, if necessary
project convention that is governing, use code style tools (e.g.
`styleR` or `Code > Reformat` in RStudio,
or [`pep8` in Python](https://swcarpentry.github.io/python-novice-gapminder/18-style/index.html#follow-standard-python-style-in-your-code),
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this particular section in the gapminder lesson mentions the google style guide for Python, it might be worth mentioning here too.

Copy link
Contributor Author

@katrinleinweber katrinleinweber Nov 19, 2019

Choose a reason for hiding this comment

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

This is a good point, because the Google style guide recommends to use the "yapf auto-formatter". So, I suggest we either add a link to it to python-novice-gapminder/18-style and don't mention the particular tools here. Duplicating content across lessons is not a good idea, IMHO. How about 2e0f8b3 ?

project convention that is governing, use code style tools (e.g.
`styleR` or `Code > Reformat` in RStudio,
or [`pep8` in Python](https://swcarpentry.github.io/python-novice-gapminder/18-style/index.html#follow-standard-python-style-in-your-code),
[ShellCheck(.net)](http://swcarpentry.github.io/shell-novice/guide/#teaching-notes),
Copy link
Contributor

Choose a reason for hiding this comment

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

Since ShellCheck doesn't appear to be a style guide, I think it is misleading to include it in this list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, but this section is about "code style tools", not "style guides" ;-)

Copy link
Contributor

Choose a reason for hiding this comment

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

ShellCheck appears to be a debugger, not a code formatter/code style tool.

Co-Authored-By: Madicken Munk <madicken.munk@gmail.com>
`htmltidy`, `perltidy`, `rubocop`, etc.) to enforce, if necessary
project convention that is governing, use code style tools (e.g.
`styleR` or `Code > Reformat` in RStudio,
or [the tools listed in our Python lesson](https://swcarpentry.github.io/python-novice-gapminder/18-style/index.html#follow-standard-python-style-in-your-code),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
or [the tools listed in our Python lesson](https://swcarpentry.github.io/python-novice-gapminder/18-style/index.html#follow-standard-python-style-in-your-code),
or `pep8` or `the google style guide`, which are mentioned [in our Python lesson](https://swcarpentry.github.io/python-novice-gapminder/18-style/index.html#follow-standard-python-style-in-your-code),

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is better to explicitly list style guides like pep8 and the google style guide here. This change forces learners to click on the link to learn what Python style tools might be available to them, and I think that's not a good practice for the lesson.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's agree to disagree on this, again. I don't think avoiding-at-all-costs for learners or instructors to click on a link (only if they want to; not "forced"!), read elsewhere, then browse back to the lesson is worth:

  1. mixing tools and guides,
  2. duplicating names here that are less relevant than the link itself, and
  3. setting future maintainers up for needing to fix a mismatch of those names, if they ever update one side of this link.

I'll not invest any more time in this. Feel free to modify the PR as you find best. I'll focus on preparing to teach a workshop.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that this will add some maintainer burden regarding your point (3), which does concern me. However, I think it's worth ensuring that the learner experience is smooth rather than that the maintainer experience is easier. In this particular case, the r and shell tools are clear and obvious in the text while the python tools are now obfuscated. I don't think that's particularly helpful to any learners that may be curious about the Python style tools.

Good luck teaching the workshop!

@munkm munkm added help wanted Looking for Contributors status:changes requested Waiting for Contributor to update PR status:in progress Contributor working on issue type:clarification Suggest change for make lesson clearer labels Nov 19, 2019
@maxim-belkin
Copy link
Contributor

I'd like to weigh in here.

What I like about this PR

  • it refers to the tools that The Carpentries actually teach
  • it provides a suggestion how to share information in a project.

Where it could be improved

  • "Code -> Reformat Code" doesn't do much -- it seems that this is a code beautifier, not style-enforcer, like styleR or pep8 (btw, pep8 is both, a style guide and a tool that was recently renamed to pycodestyle). I think "Code -> Reformat Code" is from a different category of tools so shouldn't be listed.
  • linking to Python Novice Gapminder -- Imagine telling a story and saying "...like chickens in modern era, some creatures that you can read about in another book in middle age, and dinosaurs millions of years ago...". (weird example, I know). The point is -- the middle part doesn't make sense. If we're providing examples for R, Python, and Bash -- it only makes sense to do it for every tool. So, I think a specific Python tool (style guide enforcer) has to be listed.
  • Links. In general, I think links have to point to the home pages of the linked text. Links to our lessons should be preceded with a text that reads something like this You can learn more about these tools in our lessons on [Python](link), [R](link), and [Bash](link). But this is a separate sentence and the benefits of adding it have to be reviewed.
  • change ShellCheck.Net to ShellCheck

So, in summary, my suggestions are:

  • remove "Code -> Reformat Code"
  • list pycodestyle for Python
  • remove links to SWC lessons
  • if discussions of these tools in our lessons are significant -- consider adding links to these discussions in a separate sentence.

Great work y'all!

@kekoziar
Copy link
Contributor

Hi, @katrinleinweber. This PR is old, but not stale. Are you able to update your PR taking into account the last two comments?

@kekoziar
Copy link
Contributor

I'm going to close this PR in preparation of the new workbench lesson infrastructure, but with a comment that the content is generally approved, but needed to be updated. I will leave the labels in place so it's easier to find to update.

@kekoziar kekoziar closed this Apr 28, 2023
@kekoziar kekoziar added status:waiting for response Waiting for Contributor to respond to maintainers' comments or update PR and removed status:in progress Contributor working on issue status:changes requested Waiting for Contributor to update PR help wanted Looking for Contributors labels Apr 30, 2023
zkamvar pushed a commit to swcarpentry/python-novice-gapminder that referenced this pull request May 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:waiting for response Waiting for Contributor to respond to maintainers' comments or update PR type:clarification Suggest change for make lesson clearer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants