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

[Moose] ConfigOB - grepping the boolean config settings using ConfigOB #500

Merged

Conversation

cmadjar
Copy link
Collaborator

@cmadjar cmadjar commented Oct 8, 2019

Description

In this PR, all boolean Config options are queried using the ConfigOB instead of the function in DBI.pm.

This fixes partly

)
&& (NeuroDB::DBI::getConfigSetting($this->{dbhr},'createCandidates'))
my $configOB = $this->{'configOB'};
if (!NeuroDB::MRI::subjectIDExists($subjectIDsref->{'CandID'}, $this->{dbhr})
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need to instantiate $configOB again: it was already defined on line 1585. Also, the definition of $createCandidates should go here instead of further up.

RETURN: (boolean) 1 if create_nii is set to Yes in the Config module, 0 otherwise

=cut
sub getCreateNii {
Copy link
Collaborator

@nicolasbrossard nicolasbrossard Oct 15, 2019

Choose a reason for hiding this comment

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

I think it would be a good idea to have a private function getBoolean that the other methods would call instead of "hardcoding" the logic inside each method. What do you think?

Oh and here's how you declare/used a private function:

my $privatePrintMethodRef = sub {
    my($arg) = @_;
    print "$arg.\n";
};
.
.
.
sub myPublicSayHelloMethod {
    $myPrivatePrintMethodRef->('Hello');
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree!

@cmadjar
Copy link
Collaborator Author

cmadjar commented Oct 15, 2019

@nicolasbrossard Thank you for the review! I integrated all your comments in the last commit. Please re-review :)

Copy link
Collaborator

@nicolasbrossard nicolasbrossard left a comment

Choose a reason for hiding this comment

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

Minor nitpick but otherwise looks very good!

uploadNeuroDB/NeuroDB/objectBroker/ConfigOB.pm Outdated Show resolved Hide resolved
uploadNeuroDB/NeuroDB/objectBroker/ConfigOB.pm Outdated Show resolved Hide resolved
@cmadjar
Copy link
Collaborator Author

cmadjar commented Oct 16, 2019

@nicolasbrossard Thank you! Your suggestions have been integrated in the last commit. Please re-review.

@cmadjar cmadjar merged commit cb0e7b7 into aces:minor Oct 18, 2019
@cmadjar cmadjar deleted the call_ConfigOB_across_pipeline_for_bool_configs branch October 18, 2019 14:35
@cmadjar cmadjar modified the milestones: 21.1, 22.0 Oct 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants