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

Technical limitations of how old one can be on D* #5373 #5639

Merged
merged 1 commit into from Feb 9, 2015

Conversation

@aka001
Copy link
Contributor

commented Feb 8, 2015

Please review the pull request.

@@ -35,7 +36,7 @@
= t('profiles.edit.your_birthday')

.row-fluid
= select_date profile.birthday, { :prompt => true, :default => true, :order => t('date.order'), :start_year => 2000, :end_year => 1910, :prefix => 'profile[date]' }, { :class => 'span4' }
= select_date profile.birthday, { :prompt => true, :default => true, :order => t('date.order'), :start_year => val, :end_year => 1910, :prefix => 'profile[date]' }, { :class => 'span4' }

This comment has been minimized.

Copy link
@jhass

jhass Feb 8, 2015

Member

How about putting just 13.years.ago.year here directly? See http://api.rubyonrails.org/classes/ActiveSupport/Duration.html#method-i-ago

This comment has been minimized.

Copy link
@aka001

aka001 Feb 8, 2015

Author Contributor

@jhass Thanks for a quick reply. I have updated the pr. The line 13.years.ago works.

@jhass

This comment has been minimized.

Copy link
Member

commented Feb 8, 2015

Actually it would be nice to read the minimum age from the config, like it's done in terms page and set the maximum age to 125 years, like I mentioned in #5373

@aka001

This comment has been minimized.

Copy link
Contributor Author

commented Feb 8, 2015

@jhass Okay. I am working on it.

@aka001

This comment has been minimized.

Copy link
Contributor Author

commented Feb 8, 2015

I have updated the pr. Kindly look into it. Thank you for all your help.

@@ -2,6 +2,8 @@
-# licensed under the Affero General Public License version 3 or later. See
-# the COPYRIGHT file.
- lower_limit = upper_limit_date_of_birth
- upper_limit = lower_limit_date_of_birth

This comment has been minimized.

Copy link
@jhass

jhass Feb 8, 2015

Member

There's no need for these assignments, just call the helpers directly below.


module ProfileHelper
def upper_limit_date_of_birth()
minimum_year=AppConfig.age.minimum

This comment has been minimized.

Copy link
@jhass

jhass Feb 8, 2015

Member

The key is AppConfig.settings.terms.minium_age. In this case you also want to append a .get to get rid of the proxy object. Also please use spaces around the = in assignments.

module ProfileHelper
def upper_limit_date_of_birth()
minimum_year=AppConfig.age.minimum
if AppConfig.age.minimum==nil

This comment has been minimized.

Copy link
@jhass

jhass Feb 8, 2015

Member

With the correct key, you'll see the default value is false, not nil. Also reuse the variable from above. In general don't do == nil or == false in assignments, but rely on the truthiness and falsiness rules instead.


def lower_limit_date_of_birth()
maximum_year=AppConfig.age.maximum
if AppConfig.age.maximum==nil

This comment has been minimized.

Copy link
@jhass

jhass Feb 8, 2015

Member

I don't think this needs to be configurable, just hardcode 125.years.ago.year

# the COPYRIGHT file.

module ProfileHelper
def upper_limit_date_of_birth()

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Feb 9, 2015

Omit the parentheses in defs when the method doesn't accept any arguments.

minimum_year.years.ago.year
end

def lower_limit_date_of_birth()

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Feb 9, 2015

Omit the parentheses in defs when the method doesn't accept any arguments.

def lower_limit_date_of_birth()
125.years.ago.year
end

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Feb 9, 2015

Extra empty line detected at body end.


module ProfileHelper
def upper_limit_date_of_birth()
minimum_year = AppConfig.settings.terms.minimum_age.get

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Feb 9, 2015

Use 2 (not 3) spaces for indentation.

end

def lower_limit_date_of_birth()
125.years.ago.year

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Feb 9, 2015

Use 2 (not 3) spaces for indentation.

@aka001

This comment has been minimized.

Copy link
Contributor Author

commented Feb 9, 2015

@jhass I have updated the pull request according to your comments. Kindly review it.

@aka001 aka001 force-pushed the aka001:5373_technical_limitation branch from fa1f1d9 to 8050ab8 Feb 9, 2015

@aka001

This comment has been minimized.

Copy link
Contributor Author

commented Feb 9, 2015

@houndci I have updated the pull request and squashed all of them. I hope it's fine now.
Thank you.

minimum_year = 13
else
minimum_year = minimum_year.to_i
end

This comment has been minimized.

Copy link
@jhass

jhass Feb 9, 2015

Member

Okay, just one last thing:

Let's prefer unless over if !:

unless minimum_year
  minimum_year = 13
else
  minimum_year = minimum_year.to_i
end

Let's prefer if over unless when we can swap branches:

if minimum_year
  minimum_year = minimum_year.to_i
else
  minimum_year = 13
end

Let's extract the assignment:

 minimum_year = if minimum_year
  minimum_year.to_i
else
  13
end

And reduce it to a ternary:

 minimum_year = minimum_year ?  minimum_year.to_i : 13

This comment has been minimized.

Copy link
@aka001

aka001 Feb 9, 2015

Author Contributor

Thank you for the explanation. I wasn't aware of the convention followed in diaspora.
I have updated the pull request and squashed the commits.

@aka001 aka001 force-pushed the aka001:5373_technical_limitation branch from 9623287 to 6b10527 Feb 9, 2015

@jhass jhass merged commit 6b10527 into diaspora:develop Feb 9, 2015

1 of 2 checks passed

continuous-integration/travis-ci The Travis CI build failed
Details
hound Hound has reviewed the changes.

jhass added a commit that referenced this pull request Feb 9, 2015

Merge pull request #5639 from aka001/5373_technical_limitation
Technical limitations of how old one can be on D* #5373
@jhass

This comment has been minimized.

Copy link
Member

commented Feb 9, 2015

Thanks!

@jhass jhass added this to the next-major milestone Feb 9, 2015

@RoseTheFlower

This comment has been minimized.

Copy link

commented Feb 9, 2015

Definitely looks better than the way it is now. Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.