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

Feature/flat namespace #58

Merged
merged 13 commits into from
Oct 21, 2019
Merged

Feature/flat namespace #58

merged 13 commits into from
Oct 21, 2019

Conversation

maxfischer2781
Copy link
Contributor

@maxfischer2781 maxfischer2781 commented Oct 8, 2019

This pull request flattens the usim namespace. Changes include:

  • merged usim.basics into usim,
  • adjusted documentation to a single namespace with subtopics.

This pull request closes #54, closes #20 and addresses #36.

@codecov
Copy link

codecov bot commented Oct 8, 2019

Codecov Report

Merging #58 into master will decrease coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #58      +/-   ##
==========================================
- Coverage   97.23%   97.22%   -0.01%     
==========================================
  Files          26       25       -1     
  Lines        1844     1840       -4     
  Branches      172      172              
==========================================
- Hits         1793     1789       -4     
  Misses         39       39              
  Partials       12       12

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 3f3e59d...351df13. Read the comment docs.

@maxfischer2781
Copy link
Contributor Author

I've added a documentation build for this branch to readthedocs.

@maxfischer2781 maxfischer2781 marked this pull request as ready for review October 14, 2019 13:33
@maxfischer2781
Copy link
Contributor Author

The changes to the actual usim package are minimal. All objects are now documented, but I did not write fluffy words for all sections.

I recommend finishing the documentation in a later pull request, possibly for each topic individually.

Copy link
Member

@eileen-kuehn eileen-kuehn left a comment

Choose a reason for hiding this comment

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

Looks very neat now, great!

I left some comments to improve the documentation even more :)

Comment on lines 71 to 72
A :py:class:`~usim.Scope` that is also forcefully closed
when a specific notification triggers.
Copy link
Member

Choose a reason for hiding this comment

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

A bit confusion compared to the Scope above. It might help to leave out the also. I think the description should make clear that it is about the specific notification.

Comment on lines 75 to 76
These can be inspected for their current status,
and various exceptions are unique to them.
Copy link
Member

Choose a reason for hiding this comment

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

Are the exceptions unique for the current status?
What do you think about making it into two sentences. It might help to state:

Suggested change
These can be inspected for their current status,
and various exceptions are unique to them.
Various exceptions are unique to tasks.

@eileen-kuehn
Copy link
Member

The changes to the actual usim package are minimal. All objects are now documented, but I did not write fluffy words for all sections.

I recommend finishing the documentation in a later pull request, possibly for each topic individually.

Would you add another ticket for adapting the documentation? Thanks!

@maxfischer2781
Copy link
Contributor Author

Opened #60 to track manual documentation.

@maxfischer2781
Copy link
Contributor Author

Added missing introduction sections and cleaned up the one mentioned.

Copy link
Member

@eileen-kuehn eileen-kuehn left a comment

Choose a reason for hiding this comment

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

Thanks for the adaptations. Looks good now, I only found two minor things O:)

Copy link
Member

@eileen-kuehn eileen-kuehn left a comment

Choose a reason for hiding this comment

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

Looks great now! 👍

@maxfischer2781 maxfischer2781 merged commit 61eb12e into master Oct 21, 2019
@eileen-kuehn eileen-kuehn deleted the feature/flat-namespace branch October 25, 2019 21:35
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.

Flatten uSim namespace Accessibility of CancelTask through usim API
2 participants