Skip to content

Conversation

@mlshvch
Copy link
Contributor

@mlshvch mlshvch commented Nov 4, 2021

I've completed Ruby-Lab hometask 1 with described requirements.

@StalemateInc StalemateInc self-requested a review November 4, 2021 11:29
StalemateInc
StalemateInc previously approved these changes Nov 4, 2021
Copy link

@StalemateInc StalemateInc left a comment

Choose a reason for hiding this comment

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

👍

A small side note: it’s a good practice to try and fix the styleguide offences locally before pushing to the remote to a) minimize the noise in PR; b) reduce the amount of spam from Hound. Hound is simply a bot which uses your Rubocop config, which means that you can check for offences by yourself. Just install the Rubocop locally (gem install rubocop:1.22.1) and make a habit of running it before opening the PR. This way the amount of commits with style fixes can be reduced drastically.

More tips like this one you can find in #general channel in Student’s Lab Slack workspace.

Copy link
Contributor

@ramantechart ramantechart left a comment

Choose a reason for hiding this comment

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

Fix your commit history.
Hound fixes should be wrapped in a one commit

@@ -0,0 +1,5 @@
# frozen_string_literal: true

# !/usr/bin/env ruby
Copy link
Contributor

Choose a reason for hiding this comment

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

This won't make the file executable.
Try to run $ ./hello_world.rb in the hometask folder.

Choose a reason for hiding this comment

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

Or, to be precise, that's not the only thing to make it executable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean adding information about "chmod +x /mlshvch/hometask-1/hello_world.rb" into README.md or sth else?

And as I've understood from previous comments, I need to revert state of branch to "Create hello_world.tb file" commit by means of "git revert"?

Copy link
Contributor

@ramantechart ramantechart Nov 4, 2021

Choose a reason for hiding this comment

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

@StalemateInc @mlshvch this part especially was done with typo which makes me thinking it wasn't tested locally

Copy link
Contributor

@ramantechart ramantechart Nov 4, 2021

Choose a reason for hiding this comment

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

@mlshvch
This won't make the file executable. means your file is not executable.

➜  hometask-1 git:(mlshvch-hometask-1) ✗ ./hello_world.rb
./hello_world.rb: line 5: puts: command not found

You have a typo in line 3.

Speaking of commit history, you've got a bunch of options.
Some are easy to do, some are pretty complex, but I'm going to suggest the simplest one.

In my opinion for this particular PR based on changes you've made, your commit history should look like:

- Create hello_world.rb file
- Make hello_world.rb file executable
- Added frozen_string comment to hello_world.rb file
- Fixed comments in hello_world.rb file

or a bit simpler:

- Create hello_world.rb file
- Make hello_world.rb file executable
- Fix Hound violations

But as soon as you haven't completed executable part of the task, in your case, I'd suggest to do next:

  • complete the task
  • run rubocop locally in order to avoind Hound violations
  • squash commits in one locally and it'll 1 commit history
  • push changes to the branch

@mlshvch mlshvch merged commit cc05628 into iTechArt-RubyLab:rubylab-2021-2022 Nov 8, 2021
GEK62 pushed a commit to GEK62/Ruby-Lab that referenced this pull request Nov 12, 2021
Bump rubocop version

Local run fix

Add description for hometask iTechArt-RubyLab#2 (iTechArt-RubyLab#24)

Created AndrewTehan/hometask-1 dir and hello_world.rb (iTechArt-RubyLab#21)

first commit (iTechArt-RubyLab#22)

Pivovar Andrew/hometask 1 (iTechArt-RubyLab#13)

* Create helloworld.rb

* Update helloworld.rb

* Update helloworld.rb

* Update helloworld.rb

* Update helloworld.rb

PandaGrom hometask 1 (iTechArt-RubyLab#5)

* create hello_world.rb

* fix hound issues

* make hello_world.rb file executable

Gek62 hometask 1 (iTechArt-RubyLab#12)

* First init

* Local run fix

trueananas17/hometask-1 (iTechArt-RubyLab#34)

* Create hello_world.rb

* Update hello_world.rb

* Update hello_world.rb

* Update hello_world.rb

* move # !/usr/bin/env ruby to the upper line

HOMETASK-1/init-project (iTechArt-RubyLab#23)

add hello world script

Valeriyapetrova/hometask 1 (iTechArt-RubyLab#31)

homework_1 (iTechArt-RubyLab#25)

* dz1

Co-authored-by: Roma Dunovskiy <romyashi@MacBook-Air-Roma.local>

alhimick-prog hometask 1 (iTechArt-RubyLab#20)

* first_hometask

* fix_final_newline_frozen_string_literal

Mlshvch hometask 1 (iTechArt-RubyLab#18)

* Create hello_world.rb file

* Make hello_world.rb file executable

* Fix Hound violations

hometask_1 (iTechArt-RubyLab#15)

PandaGrom hometask 2 (iTechArt-RubyLab#30)

* Complete hometask 2

* fix defect

Complete hometask-1 (iTechArt-RubyLab#6)

Complete hometask 2 (iTechArt-RubyLab#38)

Add description for hometask iTechArt-RubyLab#3 (iTechArt-RubyLab#45)

Silitski/hometask1 (iTechArt-RubyLab#47)

Hometask-1. Add hello_world.rb (iTechArt-RubyLab#49)

hometask-1 (iTechArt-RubyLab#43)

* hometask-1

* Refactor code

* Refactor code 2

* Add env ruby

add first task (iTechArt-RubyLab#41)

* add firts task

* move hometask folder into anastasi-ivl

* Update hello_world.rb

* Added empty line after magic lines

* hello_world.rb executable

Hometask 2 upload

run_cli fix
qqqwww123cdv pushed a commit to qqqwww123cdv/Ruby-Lab that referenced this pull request Nov 16, 2021
* Create hello_world.rb file

* Make hello_world.rb file executable

* Fix Hound violations
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