NotificationsYou must be signed in to change notification settings
Fork768
Star5.4k

Remove unnecessary`CheckExceptionOccurred` calls#1175

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

filmor merged 11 commits intopythonnet:masterfromamos402:remove-redundant-calls

Aug 15, 2020

Merged

Remove unnecessary`CheckExceptionOccurred` calls#1175

filmor merged 11 commits intopythonnet:masterfromamos402:remove-redundant-calls

Aug 15, 2020

Conversation

Copy link

Member

amos402 commentedJul 2, 2020

What does this implement/fix? Explain your changes.

Fro saving unnecessary costs.

Does this close any currently open issues?

...

Any other comments?

...

Checklist

Check all those that are applicable and complete.

Make sure to include one or more tests for your change
If an enhancement PR, please create docs and at best an example
Add yourself toAUTHORS
Updated theCHANGELOG

Remove unnecessaryCheckExceptionOccurred calls

03a401e

Copy link

dnfadmin commentedJul 2, 2020•
edited
Loading

All CLA requirements met.

Copy link

codecov-commenter commentedJul 2, 2020•
edited
Loading

Codecov Report

Merging#1175 intomaster willnot change coverage.
The diff coverage isn/a.

@@           Coverage Diff           @@##           master    #1175   +/-   ##=======================================  Coverage   86.25%   86.25%           =======================================  Files           1        1             Lines         291      291           =======================================  Hits          251      251             Misses         40       40

Flag	Coverage Δ
#setup_linux	`64.94% <ø> (ø)`
#setup_windows	`72.50% <ø> (ø)`

Flags with carried forward coverage won't be shown.Click here to find out more.

Continue to review full report at Codecov.

Legend -Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing data
Powered byCodecov. Last update2d2b297...8ca0a62. Read thecomment docs.

lostmsu requested changes

Jul 2, 2020

View reviewed changes

src/runtime/NewReference.cs OutdatedShow resolvedHide resolved

src/runtime/pythonexception.cs OutdatedShow resolvedHide resolved

src/runtime/converter.cs Outdated

		}
		doubledd=Runtime.PyFloat_AsDouble(op);
		Runtime.CheckExceptionOccurred();
		if(dd==-1.0)

Copy link

Member

lostmsuJul 2, 2020

Choose a reason for hiding this comment

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

Knowing the funny behavior of floating-point comparisons, it would be good to have two test cases for this conversion passing and failing.

amos402 added7 commits

July 29, 2020 22:46

Dispose safety

950676f

Refactor Converter.ToPrimitive

88f2cfa

str to int raisesValueError

0e4668c

Test convert error for float types

9845e98

Convert error for UInt32

97f0009

* Ignore package index

838dfc2

* Install pycparser on Windows ci

BorrowedReference instead ofin NewReference

d9628f7

filmor reviewed

Jul 30, 2020

View reviewed changes

appveyor.yml Outdated

		install:
		-python -m pip install -U pip
		-pip install --upgrade -r requirements.txt --quiet
		-pip install install pycparser --quiet

Copy link

Member

filmorJul 30, 2020

Choose a reason for hiding this comment

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

This looks wrong.

Copy link

MemberAuthor

amos402Jul 30, 2020

Choose a reason for hiding this comment

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

Yes, apparently it's wrong, never thought it works.😂
This line is due torequirements.txt declared won'tpycparser be install on windows, butsetup process need it.