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

Fix NoMethodError for --pr option (caused by module_options = nil) / introduce --noop #188

Merged
merged 5 commits into from
Aug 11, 2020

Conversation

bittner
Copy link
Contributor

@bittner bittner commented Aug 6, 2020

Adds --noop support for the --pr option.

Adds feature tests for both GitHub PRs and GitLab MRs to prove the bug reported with issue #187.

Fixes #187

The newly added tests prove the bug reported with issue voxpupuli#187.
This change also adds --noop support for the --pr option.
@bittner bittner added the bug label Aug 6, 2020
@bittner bittner marked this pull request as draft August 6, 2020 17:18
@bittner bittner changed the title WIP: Fix NoMethodError for --pr option (caused by module_options = nil) Fix NoMethodError for --pr option (caused by module_options = nil) Aug 6, 2020
@bastelfreak bastelfreak requested review from raphink and ekohl August 9, 2020 15:13
@bittner bittner requested a review from raphink August 10, 2020 15:19
@bittner bittner force-pushed the bugfix/pr-module_options-nil branch from bb26f94 to d75e971 Compare August 10, 2020 16:05
@bittner bittner marked this pull request as ready for review August 10, 2020 16:09
@bittner
Copy link
Contributor Author

bittner commented Aug 10, 2020

This should be ready for merging now. Can you take a look again? Thanks!

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

I wonder if the following pattern would be easier:

do_prep_work

if options[:noop]
  $stdout.puts "Using no-op. Would do real work"
  return
end

do_real_work

I don't know the answer - there are really good arguments for both.

lib/modulesync.rb Outdated Show resolved Hide resolved
lib/modulesync.rb Show resolved Hide resolved
@bittner bittner force-pushed the bugfix/pr-module_options-nil branch from 4541081 to c948011 Compare August 11, 2020 07:38
@bittner
Copy link
Contributor Author

bittner commented Aug 11, 2020

I wonder if the following pattern would be easier:

I have applied your suggested changes to the GitLab pr module. That makes the code structure flatter, which is better. Nice, thank you!

In the GitHub pr module the code is different: The code, which sets the labels, needs to run afterwards and has a separate :noop clause. It might be a good idea to align the two implementations, e.g. while fixing issue #176, so that code could also benefit from a flatter structure.

@bittner bittner requested a review from ekohl August 11, 2020 07:57
@bittner
Copy link
Contributor Author

bittner commented Aug 11, 2020

Any more corrections needed? Otherwise, I'd be happy if we could merge the changes. 🤞

@raphink
Copy link
Member

raphink commented Aug 11, 2020

I wonder if the following pattern would be easier:

do_prep_work

if options[:noop]
  $stdout.puts "Using no-op. Would do real work"
  return
end

do_real_work

I don't know the answer - there are really good arguments for both.

I was going to say the same actually. I prefer short circuits for this kind of logic.

@bittner bittner force-pushed the bugfix/pr-module_options-nil branch from c948011 to 3e7685e Compare August 11, 2020 13:44
@bittner bittner requested a review from raphink August 11, 2020 13:48
Align implementation for GitHub PRs and GitLab MRs
@bittner bittner force-pushed the bugfix/pr-module_options-nil branch from 3e7685e to ebc888f Compare August 11, 2020 15:10
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

@raphink I think this looks good, but since reviewed it too I'll let you do the final review.

@raphink
Copy link
Member

raphink commented Aug 11, 2020

Ok for me then

@bittner bittner merged commit f20cfa5 into voxpupuli:master Aug 11, 2020
@bittner bittner deleted the bugfix/pr-module_options-nil branch August 11, 2020 20:39
@bittner
Copy link
Contributor Author

bittner commented Aug 11, 2020

Thank you @raphink and @ekohl! 🥇

@bastelfreak bastelfreak changed the title Fix NoMethodError for --pr option (caused by module_options = nil) Fix NoMethodError for --pr option (caused by module_options = nil) / introduce --noop Aug 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GitLab MR: undefined method `[]' for nil:NilClass (NoMethodError)
3 participants