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

1.0.alpha.4 (Sourcery refactored) #214

Merged
merged 1 commit into from
Jan 14, 2023
Merged

1.0.alpha.4 (Sourcery refactored) #214

merged 1 commit into from
Jan 14, 2023

Conversation

sourcery-ai[bot]
Copy link
Contributor

@sourcery-ai sourcery-ai bot commented Jan 14, 2023

Pull Request #213 refactored by Sourcery.

If you're happy with these changes, merge this Pull Request using the Squash and merge strategy.

NOTE: As code is pushed to the original Pull Request, Sourcery will
re-run and update (force-push) this Pull Request with new refactorings as
necessary. If Sourcery finds no refactorings at any point, this Pull Request
will be closed automatically.

See our documentation here.

Run Sourcery locally

Reduce the feedback loop during development by using the Sourcery editor plugin:

Review changes via command line

To manually merge these changes, make sure you're on the develop branch, then run:

git fetch origin sourcery/develop
git merge --ff-only FETCH_HEAD
git reset HEAD^

Help us improve this pull request!

@sourcery-ai sourcery-ai bot requested a review from cleder January 14, 2023 03:03
@sourcery-ai
Copy link
Contributor Author

sourcery-ai bot commented Jan 14, 2023

Sourcery Code Quality Report

✅  Merging this PR will increase code quality in the affected files by 0.10%.

Quality metrics Before After Change
Complexity 4.77 ⭐ 4.70 ⭐ -0.07 👍
Method Length 51.82 ⭐ 51.47 ⭐ -0.35 👍
Working memory 7.05 🙂 6.95 🙂 -0.10 👍
Quality 72.50% 🙂 72.60% 🙂 0.10% 👍
Other metrics Before After Change
Lines 323 315 -8
Changed files Quality Before Quality After Quality Change
fastkml/mixins.py 83.59% ⭐ 81.58% ⭐ -2.01% 👎
fastkml/views.py 69.85% 🙂 70.79% 🙂 0.94% 👍

Here are some functions in these files that still need a tune-up:

File Function Complexity Length Working Memory Quality Recommendation
fastkml/views.py _AbstractView.etree_element 11 🙂 260 ⛔ 9 🙂 46.23% 😞 Try splitting into smaller methods
fastkml/views.py _AbstractView.from_element 8 ⭐ 234 ⛔ 7 🙂 54.91% 🙂 Try splitting into smaller methods
fastkml/views.py _AbstractView.__init__ 2 ⭐ 102 🙂 13 😞 61.53% 🙂 Extract out complex expressions
fastkml/views.py Camera.__init__ 0 ⭐ 81 🙂 14 😞 65.09% 🙂 Extract out complex expressions
fastkml/views.py LookAt.__init__ 0 ⭐ 81 🙂 14 😞 65.09% 🙂 Extract out complex expressions

Legend and Explanation

The emojis denote the absolute quality of the code:

  • ⭐ excellent
  • 🙂 good
  • 😞 poor
  • ⛔ very poor

The 👍 and 👎 indicate whether the quality has improved or gotten worse with this pull request.


Please see our documentation here for details on how these metrics are calculated.

We are actively working on this report - lots more documentation and extra metrics to come!

Help us improve this quality report!

@what-the-diff
Copy link

what-the-diff bot commented Jan 14, 2023

  • The time_stamp property of the TimeMixin class was changed to use a ternary operator instead of an if statement.
  • The begin and end properties were also updated with similar changes as above, using ternary operators in place of if statements.
  • In altitude_mode setter method, we see that it is now checking for membership within a set rather than comparing against strings directly (which would be slower). This change makes sense because there are only three possible values for this field so storing them in a hash table will make lookups faster than linear search through string comparisons each time the value needs to be checked or modified by other methods/functions on this object type (Views).
  • Lastly, we can see that some code duplication has been removed from the Views class's from_element() function where previously two separate find operations were being performed when looking up elements based on namespace prefixes; one check was made specifically for gx:altitudeMode while another looked at kml:altitudeMode but both had identical logic after finding their respective element tags - they simply assigned those found elements' text content back into self._altitude_mode without any further processing needed since all valid options have already been accounted for during initialization via init(). Now these checks are combined into one lookup operation which saves us some cycles here! :)

@ghost
Copy link

ghost commented Jan 14, 2023

👇 Click on the image for a new way to code review
  • Make big changes easier — review code in small groups of related files

  • Know where to start — see the whole change at a glance

  • Take a code tour — explore the change with an interactive tour

  • Make comments and review — all fully sync’ed with github

    Try it now!

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map Legend

@codecov
Copy link

codecov bot commented Jan 14, 2023

Codecov Report

Base: 92.94% // Head: 92.97% // Increases project coverage by +0.03% 🎉

Coverage data is based on head (48673e1) compared to base (b2cfa39).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #214      +/-   ##
===========================================
+ Coverage    92.94%   92.97%   +0.03%     
===========================================
  Files           26       26              
  Lines         4421     4414       -7     
===========================================
- Hits          4109     4104       -5     
+ Misses         312      310       -2     
Impacted Files Coverage Δ
fastkml/mixins.py 91.83% <100.00%> (+0.92%) ⬆️
fastkml/views.py 89.16% <100.00%> (+0.43%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

if self._timestamp is not None:
return self._timestamp.timestamp
return None
return self._timestamp.timestamp if self._timestamp is not None else None
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Function TimeMixin.time_stamp refactored with the following changes:

Comment on lines -53 to +51
if self._timespan is not None:
return self._timespan.begin
return None
return self._timespan.begin if self._timespan is not None else None
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Function TimeMixin.begin refactored with the following changes:

Comment on lines -71 to +67
if self._timespan is not None:
return self._timespan.end
return None
return self._timespan.end if self._timespan is not None else None
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Function TimeMixin.end refactored with the following changes:

Comment on lines -158 to +159
if mode in ("relativeToGround", "clampToGround", "absolute"):
self._altitude_mode = str(mode)
if mode in {"relativeToGround", "clampToGround", "absolute"}:
self._altitude_mode = mode
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Function _AbstractView.altitude_mode refactored with the following changes:

Comment on lines -184 to +186
if altitude_mode is not None:
self.altitude_mode = altitude_mode.text
else:
if altitude_mode is None:
altitude_mode = element.find(f"{gx.NS}altitudeMode")
self.altitude_mode = altitude_mode.text
self.altitude_mode = altitude_mode.text
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Function _AbstractView.from_element refactored with the following changes:

@cleder cleder merged commit ccf1d1a into develop Jan 14, 2023
@cleder cleder deleted the sourcery/develop branch January 14, 2023 03:31
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.

1 participant