Skip to content

bpo-39479:Add math.lcm() function: Least Common Multiple #18547

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

Merged
merged 18 commits into from
Feb 19, 2020
Merged

bpo-39479:Add math.lcm() function: Least Common Multiple #18547

merged 18 commits into from
Feb 19, 2020

Conversation

ananthan-123
Copy link
Contributor

@ananthan-123 ananthan-123 commented Feb 18, 2020

@ananthan-123
Copy link
Contributor Author

This PR is created due to some problems caused in the repository.
#18328

@ananthan-123
Copy link
Contributor Author

ananthan-123 commented Feb 18, 2020

It is showing an error

1 test failed:
test_importlib
Is it because of any changes made by me or not?

@mdickinson
Copy link
Member

mdickinson commented Feb 18, 2020

This PR is created due to some problems caused in the repository.

I recommend that you investigate Git branching: instead of making all your commits on the master branch of your fork, create a dedicated branch for the work you're doing, and make the pull request from that. Apart from anything else, that will enable you to work on multiple independent issues at once, in different branches of your fork. See the developer guide for more.

@ananthan-123
Copy link
Contributor Author

Thanks for the information.
can you tell me what is the problem in the code?

@ananthan-123
Copy link
Contributor Author

======================================================================
FAIL: test_case_insensitivity (test.test_importlib.extension.test_case_sensitivity.Frozen_ExtensionModuleCaseSensitivityTest)

Traceback (most recent call last):
File "/Users/runner/runners/2.164.0/work/cpython/cpython/Lib/test/test_importlib/extension/test_case_sensitivity.py", line 36, in test_case_insensitivity
self.assertTrue(hasattr(loader, 'load_module'))
AssertionError: False is not true

======================================================================
FAIL: test_case_insensitivity (test.test_importlib.extension.test_case_sensitivity.Source_ExtensionModuleCaseSensitivityTest)

Traceback (most recent call last):
File "/Users/runner/runners/2.164.0/work/cpython/cpython/Lib/test/test_importlib/extension/test_case_sensitivity.py", line 36, in test_case_insensitivity
self.assertTrue(hasattr(loader, 'load_module'))
AssertionError: False is not true

======================================================================
FAIL: test_insensitive (test.test_importlib.source.test_case_sensitivity.Frozen_CaseSensitivityTestPEP302)


Traceback (most recent call last):
File "/Users/runner/runners/2.164.0/work/cpython/cpython/Lib/test/test_importlib/source/test_case_sensitivity.py", line 57, in test_insensitive
self.assertIsNotNone(insensitive)
AssertionError: unexpectedly None

@ananthan-123
Copy link
Contributor Author

Above is the error showing.

Copy link
Member

@mdickinson mdickinson left a comment

Choose a reason for hiding this comment

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

I've left a few comments.

The macOS failures are unrelated, and are described in https://bugs.python.org/issue38691. Once #18553 is merged, you should be able to merge origin/master into your branch to get the CI to pass.

@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@bedevere-bot
Copy link

Thanks for making the requested changes!

@mdickinson: please review the changes made to this pull request.

@mdickinson
Copy link
Member

@ananthan-123:

I have made the requested changes

Not quite. :-) Please could you address my comment about alphabetical order in Misc/ACKS?

Copy link
Member

@mdickinson mdickinson left a comment

Choose a reason for hiding this comment

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

There's still one unaddressed comment from the last change request.

@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@ananthan-123
Copy link
Contributor Author

I have made the requested changes; please review again

@bedevere-bot
Copy link

Thanks for making the requested changes!

@mdickinson: please review the changes made to this pull request.

@mdickinson
Copy link
Member

@ananthan-123 Please can you investigate the failure on Travis CI? I think you may need to re-run make clinic locally and push the changes that result from that. It would also be worth merging the upstream master into this branch if you haven't already done so.

(As a general rule, it's good to try to get all the CI checks passing before asking for review.)

@ananthan-123
Copy link
Contributor Author

Hurray!The test is passed

@ananthan-123
Copy link
Contributor Author

I have made the requested changes; please review again

@bedevere-bot
Copy link

Thanks for making the requested changes!

@mdickinson: please review the changes made to this pull request.

@codecov
Copy link

codecov bot commented Feb 19, 2020

Codecov Report

Merging #18547 into master will increase coverage by 1.01%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           master   #18547       +/-   ##
===========================================
+ Coverage   82.11%   83.13%    +1.01%     
===========================================
  Files        1956     1571      -385     
  Lines      589382   414787   -174595     
  Branches    44456    44458        +2     
===========================================
- Hits       483979   344832   -139147     
+ Misses      95751    60332    -35419     
+ Partials     9652     9623       -29     
Impacted Files Coverage Δ
Lib/distutils/tests/test_bdist_rpm.py 30.00% <0.00%> (-65.00%) ⬇️
Lib/distutils/command/bdist_rpm.py 7.63% <0.00%> (-56.88%) ⬇️
Lib/test/test_urllib2net.py 76.92% <0.00%> (-13.85%) ⬇️
Lib/test/test_smtpnet.py 78.57% <0.00%> (-7.15%) ⬇️
Lib/ftplib.py 63.85% <0.00%> (-6.06%) ⬇️
Lib/test/test_ftplib.py 87.11% <0.00%> (-4.72%) ⬇️
Tools/scripts/db2pickle.py 17.82% <0.00%> (-3.97%) ⬇️
Tools/scripts/pickle2db.py 16.98% <0.00%> (-3.78%) ⬇️
Lib/test/test_socket.py 71.94% <0.00%> (-3.77%) ⬇️
Lib/test/test_asyncio/test_base_events.py 91.84% <0.00%> (-3.30%) ⬇️
... and 431 more

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 5d38517...d3edc08. Read the comment docs.

Copy link
Member

@mdickinson mdickinson left a comment

Choose a reason for hiding this comment

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

LGTM

@mdickinson mdickinson merged commit f2ee21d into python:master Feb 19, 2020
@bedevere-bot
Copy link

@mdickinson: Please replace # with GH- in the commit message next time. Thanks!

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.

4 participants