Skip to content

builtin: Implement min/max builtin function #48

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom
Merged
merged 1 commit into from
Feb 3, 2019
Merged

builtin: Implement min/max builtin function #48

merged 1 commit into from
Feb 3, 2019

Conversation

corona10
Copy link
Collaborator

@corona10 corona10 commented Jan 6, 2019 •
edited
Loading

  • Implement builtin min/max function
  • Support the keyword-only option for ParseTupleAndKeywords

@codecov-io
Copy link

codecov-io commented Jan 6, 2019 •
edited
Loading

Codecov Report

Merging #48 into master will increase coverage by 0.06%.
The diff coverage is 74.41%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #48      +/-   ##
==========================================
+ Coverage   66.02%   66.09%   +0.06%     
==========================================
  Files          58       58              
  Lines       10246    10332      +86     
==========================================
+ Hits         6765     6829      +64     
- Misses       3005     3017      +12     
- Partials      476      486      +10
Impacted Files Coverage Δ
py/args.go 52.94% <16.66%> (-2.27%) ⬇️
builtin/builtin.go 78.76% <78.75%> (-0.01%) ⬇️

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 95617b7...f3b9939. Read the comment docs.

@corona10 corona10 requested a review from ncw January 6, 2019 07:19
@corona10
Copy link
Collaborator Author

corona10 commented Jan 6, 2019

@ncw PTAL

@corona10
Copy link
Collaborator Author

@ncw gentle ping~

@ncw
Copy link
Collaborator

ncw commented Jan 11, 2019

Sorry, I got half way through reviewing this and got distracted!

ncw
ncw reviewed Jan 11, 2019
View reviewed changes
Copy link
Collaborator

@ncw ncw left a comment

Choose a reason for hiding this comment

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

See inline :-)

@corona10
Copy link
Collaborator Author

@ncw
Looks like we have to implement keyword only arugment for ParseTupleAndKeywordsby supporting '$'.
I will update this PR soon

@corona10 corona10 force-pushed the builtin_min_max branch 2 times, most recently from ef5aebe to 7cc4cdb Compare January 30, 2019 07:06
@corona10
Copy link
Collaborator Author

@ncw

  • Now I've implemented full features of builtin min/max function
  • Also I've implemented for keyword only options for ParseTupleAndKeywords

PTAL

@corona10
builtin: Implement min/max builtin function
f3b9939 
This CL implementation is not 100% accurate since,
keyfunc and default value parameter is not supported.

We can support it later CL.
@corona10
Copy link
Collaborator Author

corona10 commented Feb 1, 2019

@ncw gentle ping~ :)

ncw
ncw approved these changes Feb 3, 2019
View reviewed changes
Copy link
Collaborator

@ncw ncw left a comment

Choose a reason for hiding this comment

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

Hey that looks great now :-)

Apologies for the delay!

Please merge.

@corona10 corona10 merged commit 05fb6f3 into go-python:master Feb 3, 2019
@corona10 corona10 deleted the builtin_min_max branch February 3, 2019 21:12
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@ncw ncw ncw approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@corona10 @codecov-io @ncw