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

Cleanup manifest #38

Merged
merged 13 commits into from
Nov 30, 2021
Merged

Cleanup manifest #38

merged 13 commits into from
Nov 30, 2021

Conversation

nclack
Copy link
Collaborator

@nclack nclack commented Nov 25, 2021

closes #17

This removes several fields. These include

  • contributions
  • license (keep)
  • categories
  • preview
  • private

I also went through and tried to remove comments, TODO's, add field documentation, etc.

The EXAMPLE folder was removed; it contained an old example.

Please note TODO's and FIXME's that you'd like to keep.

@codecov
Copy link

codecov bot commented Nov 25, 2021

Codecov Report

Merging #38 (9964f72) into main (6227387) will increase coverage by 0.28%.
The diff coverage is 100.00%.

❗ Current head 9964f72 differs from pull request most recent head 01e03be. Consider uploading reports for the commit 01e03be to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main      #38      +/-   ##
==========================================
+ Coverage   87.53%   87.82%   +0.28%     
==========================================
  Files          22       22              
  Lines        1051     1043       -8     
==========================================
- Hits          920      916       -4     
+ Misses        131      127       -4     
Impacted Files Coverage Δ
npe2/_command_registry.py 87.80% <ø> (ø)
npe2/_from_npe1.py 92.26% <100.00%> (+0.64%) ⬆️
npe2/_plugin_manager.py 92.04% <100.00%> (ø)
npe2/manifest/schema.py 77.63% <100.00%> (+0.44%) ⬆️

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 6227387...01e03be. Read the comment docs.

Comment on lines 30 to 31
# TODO: validate argumemnts and type constraints
# possibly wrap command in a type validator?
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm in favor of handling this w #29 right now; the cli tool can do some initial validation to type check.

Comment on lines +320 to +323
# When the list of extensions for the writer doesn't match the
# extension in the filename, keep searching.

# Nothing got found
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this fixes some broken tests I was getting on the napari side. napari's CI misses these at the moment because npe2 isn't installed when the test suite is running.

Comment on lines +53 to +54
description="The name of the plugin. Should correspond to the python "
"package name for this plugin.",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is related to the discussion in #37. I think the best approach is to require the name to be the package name. I haven't changed any of the validation so #37 is still an issue.

@tlambert03
Copy link
Collaborator

hmm, i didn't particularly mind most of the comments that you removed here. I think many of them are still "open questions" that haven't received any particularly special attention that warrants comment removal. or are perhaps just generally useful in terms of where the code was inspired (such as the vscode comments).
Can you perhaps return the comments and just focus on the fields that you want to modify?

tlambert03 and others added 5 commits November 27, 2021 09:33
@nclack
Copy link
Collaborator Author

nclack commented Nov 28, 2021

Can you perhaps return the comments and just focus on the fields that you want to modify?

Yes. Let me know if I missed anything.

I'm keeping this in sync w #40

@tlambert03
Copy link
Collaborator

tlambert03 commented Nov 30, 2021

why are you removing license (like other fields, it could certainly be auto-populated by package metadata... but does it need to be removed?)

Copy link
Collaborator

@tlambert03 tlambert03 left a comment

Choose a reason for hiding this comment

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

outside of license, this is fine with me. Can we leave that discussion for another PR?

I think we generally need to consider the process of taking data from setup.cfg a little better. Let's keep it independent of "cleanup"

@nclack
Copy link
Collaborator Author

nclack commented Nov 30, 2021

license

added back.

need to consider the process of taking data from setup.cfg a little better

agreed. I also like the idea of separating that discussion from this issue.

@tlambert03 tlambert03 merged commit 948fee8 into napari:main Nov 30, 2021
@nclack nclack deleted the nclack/issue17 branch November 30, 2021 21:02
@tlambert03 tlambert03 added the refactor neither a new feature, nor a bug fix, just refactoring label Dec 15, 2021
@tlambert03 tlambert03 mentioned this pull request Aug 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor neither a new feature, nor a bug fix, just refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove some manifest fields
2 participants