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

Return long value from int filter if over max int length. #315

Merged
merged 2 commits into from
Mar 25, 2019

Conversation

mattcoley
Copy link
Collaborator

@mattcoley mattcoley commented Mar 25, 2019

Int is unbounded in python so trying to come up with a solution to bring Jinjava |int closer to parity with Jinja. Here we are returning a long value instead of a int if the value is above the maximum allowed for ints. This should fix most issues where people are trying to convert long IDs into numbers using the |int filter. Some thought is needed to figure out a better long term solution for higher values than what long supports.

@mattcoley mattcoley requested a review from boulter March 25, 2019 20:11
@codecov-io
Copy link

codecov-io commented Mar 25, 2019

Codecov Report

Merging #315 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #315      +/-   ##
============================================
+ Coverage     71.26%   71.28%   +0.01%     
- Complexity     1559     1561       +2     
============================================
  Files           239      239              
  Lines          4907     4910       +3     
  Branches        790      790              
============================================
+ Hits           3497     3500       +3     
  Misses         1127     1127              
  Partials        283      283
Impacted Files Coverage Δ Complexity Δ
...java/com/hubspot/jinjava/lib/filter/IntFilter.java 100% <100%> (ø) 10 <8> (+2) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1d475ba...254192e. Read the comment docs.

@boulter
Copy link
Contributor

boulter commented Mar 25, 2019

would there be harm in always returning a long from the int filter if in python |int means any whole number?

@mattcoley
Copy link
Collaborator Author

Not sure what the implications are for always returning long so I am trying to be safe by only returning long if the value overflows.

@mattcoley
Copy link
Collaborator Author

Filters/functions that don't do type conversions (i.e. advanced filters) may be affected by always returning a long.

@mattcoley mattcoley merged commit a4846c5 into master Mar 25, 2019
@mattcoley mattcoley deleted the use-long-int branch March 25, 2019 20:50
@boulter
Copy link
Contributor

boulter commented Mar 25, 2019

Filters/functions that don't do type conversions (i.e. advanced filters) may be affected by always returning a long.

I'd say it would be more likely to create problems if the returned type varies by input. Users won't expect they'll sometimes get longs.

@mattcoley
Copy link
Collaborator Author

I am going to fix type conversion in a few other places then make this always return long.

@mattcoley
Copy link
Collaborator Author

The only issue I can think of is if you explicitly check for the type: type(num) == 'int'

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