-
Notifications
You must be signed in to change notification settings - Fork 0
refactor: restructure a little and add tests #2
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Henry Schreiner <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #2 +/- ##
===========================================
+ Coverage 29.75% 85.71% +55.96%
===========================================
Files 10 11 +1
Lines 121 140 +19
===========================================
+ Hits 36 120 +84
+ Misses 85 20 -65
|
aba081e
to
64cf9ed
Compare
Signed-off-by: Henry Schreiner <[email protected]>
64cf9ed
to
023f72f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should add/refactor docs
in this PR as well?
@click.pass_context | ||
def init(ctx: click.Context) -> None: # noqa: ARG001 | ||
""" | ||
Add scikit-build to an existing project |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Existing CMake or Python project? Or have to have both?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not totally sure. :) I was thinking it could be like new but not overwrite existing files, and maybe adapt to some extent to what is already written.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CMake wise it could ve tricky as everyone is having a different format. We would need a build-dir if we want to do some parsing to find the libraries that link to python-devel. Or we could add_subdirectory
with an example path.
Python-side though should be easier to introspect, but custom setup.py
might be harder to figure out the intent?
Or we could assume the project is either pure-python or pure-cmake for this case? That could be the simplest to support.
Good question, I'm fine either way. |
@@ -10,6 +10,7 @@ def __dir__() -> list[str]: | |||
|
|||
|
|||
@click.command() | |||
@click.help_option("--help", "-h") | |||
@click.pass_context | |||
def metadata(ctx: click.Context) -> None: # noqa: ARG001 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking, this might make more sense to be dist_info
and rename the dynamica_metadata to metadata
? Are there any metadata outside of dist-info
that might make it confusing?
Some more refactoring. We don't have a public API, so leaving everything inside
__main__
for now.Add support for
-h
in addition to--help
.Also stubbing out
new
andinit
.