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

Executable tutorial Submission: Integrating Code Climate in developers workflow with shell scripts and docker #1395

Merged
merged 16 commits into from
Apr 29, 2021

Conversation

johennin
Copy link

@johennin johennin commented Apr 27, 2021

Original PR proposal can be found here: #1278.

The tutorial on Medium can be found here.

Easter Egg

It might be hard to find at first so here is a hint: Check all the characters in the links!

@johennin
Copy link
Author

@Atema your time to shine!

@Atema
Copy link

Atema commented Apr 27, 2021

@Atema your time to shine!

Great! I'll send you my feedback tomorrow morning

@Atema
Copy link

Atema commented Apr 28, 2021

Alright @johennin and @LeeBadal, here you go:

Feedback

Overall

Your tutorial explains two different concepts: shell scripts and Code Climate, which is nice, because the reader gets to learn a lot, but can also be a bit confusing: make sure to clearly separate which of the two does what!

This may not be something that you can integrate into the tutorial (although it would be nice if you could elaborate a bit on alternative solutions), but it's not really clear to me what the advantages are of using a shell script over automation tools that are made for exactly this (for example, Gradle, Maven, or Grunt). You mention the importance of having an easy style guide with one command to run (test.sh), but this is often already offered in the shape of ./gradlew build, or (in the case in your background story, probably together with a few other commands) ./gradlew lint.

Bit short. The tutorial feels a bit on the short side (probably less than 10 minutes). You could expand a bit on the possibilities of Code Climate, e.g. by showing plugins like PyLint, or by adding something about the online dashboard/overviews they provide (more here).

Background story

Looks good! The background story gives a nice context to the tutorial.

Introduction

The section gives a good introduction to what will happen in the tutorial. However, I'd like to see a bit more information about Code Climate, as I think most readers will not be familiar with the tool. It could also be made a bit clearer that we will do two different things: creating a shell script ánd
configuring Code Climate (at the moment, it sounds like Code Climate is used to run the dummy python tests). I think a good option would be to turn things around: start with the shell script, and mention Code Climate as one of the things that will be included in the script.

Step 1

Clear & straightforward (it could be marked as an optional step, as everything is currently run locally).

Step 2

Clear & straightforward. Could be combined with step 1 (clone repository, and open it) and step 3 (create test.sh).

Step 3

Introduction. It would be nice to have a short introduction to the file, so that you immediately know what's going on. Very much like you did in earlier in the tutorial for all files, just to remind the reader again.

The code. I'm not sure whether you wrote this script yourself, or whether you got it from Code Climate (if the latter, maybe take these with a grain of salt), but I've got a few technical things.

Engine install. According to their instructions under Project setup on https://github.com/codeclimate/codeclimate, codeclimate engines:install is recommended to be run for every new project, and after changing the configuration files. In your script, this command is only run the first time codeclimate is installed.

Using subshell. By writing the commands inside parentheses, they are run in a subshell, so you don't need to cd back and forth. However, I'm not sure everyone is familiar with the notation. (More info: https://tldp.org/LDP/abs/html/subshells.html).

Codeclimate install check. Currently, you check whether the directory codeclimate-master exists. Your code, however, installs codeclimate globally (using sudo make install), and you should check whether the binary /usr/local/bin/codeclimate exists (the user can remove the folder, but codeclimate will still be installed).

Rewrite. Implementing these changes, you could rewrite to something like:

#!/usr/bin/env bash

if [ ! -f "/usr/local/bin/codeclimate" ]
then
	curl -L https://github.com/codeclimate/codeclimate/archive/master.tar.gz | tar xvz -C /home/$USER/
	(cd /home/$USER/codeclimate-* && sudo make install)
fi

codeclimate engines:install
python3 tests/test.py
codeclimate analyze

Line numbers. It would be nice to be able to copy the code snippet without having to navigate to GitHub. You could embed a gist with the snippet (see https://blog.medium.com/yes-we-get-the-gist-1c2a27cdfc22), which would also provide syntax highlighting, making things a bit easier to read. (I don't think there's a dark mode, so it may not fit in well with the rest).

Line-by-line explainations. Nice to show precisely what happens, especially for people not really familiar with shell scripts. You could expand a bit further, and explain each line instead of combining lines.

Step 4

Introduction. Same as for the previous step, a short introduction to the file to remind the reader would be nice.

Explainations. Code Climate is one of the main focuses of your tutorial, so I would expect more information about how its config files work. Currently, the config is quite long and even contains a section that's commented out. You could instead write your own config file, that demonstrates what Code Climate is capable of, and explain the options you include.

Step 5

Straightforward & clear.

Step 6

Straightforward & clear. You should update the the string that's printed (either here or at the other places), as it's not consistent with the version in the GitHub repo and the screenshot in step 8.

Step 7

Straightforward. You could expand a bit by adding some more different types of issues, and linking them back to your config file in step 4.

Step 8

Again, "hello tests" is not consistent with step 6. Otherwise, this step is clear. Maybe you could mention to make sure that the Docker daemon is running? It gave me some problems the first time I ran the script.

Step 9

Nice!

Conclusion

Looks good! It would be nice if you expanded a bit more on the "many ways of automating processes like these" and their pros and cons.

Easter egg

Was very difficult to find, and took me a while to get what your hint meant.

@Atema
Copy link

Atema commented Apr 28, 2021

Let me know if you have any questions :)

@johennin
Copy link
Author

Let me know if you have any questions :)

Make sure to create a PR for feedback submission!

@LeeBadal
Copy link

@Atema Thanks for the feedback, we changed it a bit. Specifically explaining the config options more detailed, matching the output with the image and rewriting the script.

@khaes-kth We are ready for a review/merge.

@khaes-kth
Copy link

@LeeBadal Please make list the changes you made after the feedback in your README file.

@khaes-kth
Copy link

Also, I think you should not change the contributions/executable-tutorial/README.md file.

@khaes-kth khaes-kth self-assigned this Apr 28, 2021
@khaes-kth khaes-kth added final_submission The final submission of a task tutorial One of the task categories listed in README.md labels Apr 28, 2021
@LeeBadal
Copy link

@khaes-kth

  • Added more detail about the script in step 2.
  • Added a gist link and rewrote the description in step 3
  • Rewrote the script the look for installation rather than downloaded directory.
  • Added a section called "Explaining the config file"
  • Rewrote the code in step 6 to match the image in step 7

Copy link
Author

@johennin johennin left a comment

Choose a reason for hiding this comment

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

Removed the problem README.me file from the commit, @khaes-kth

@khaes-kth khaes-kth merged commit 1548f7c into KTH:2021 Apr 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
final_submission The final submission of a task tutorial One of the task categories listed in README.md
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants