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

feat: allow running commands with package create and package update with an entry point of package to app.py #243

Merged
merged 12 commits into from
Jan 5, 2025

Conversation

bobleesj
Copy link
Collaborator

@bobleesj bobleesj commented Jan 4, 2025

Closes #236

Tested:

Screenshot 2025-01-04 at 1 01 19 PM

@bobleesj bobleesj changed the title feat: allow running commands with package create and package update with an entry point of package feat: allow running commands with package create and package update with an entry point of package to app.py Jan 4, 2025
@bobleesj
Copy link
Collaborator Author

bobleesj commented Jan 4, 2025

@sbillinge ready for review

@bobleesj
Copy link
Collaborator Author

bobleesj commented Jan 4, 2025

@sbillinge (codecov failing, perhaps for the same reason in #242 (comment))

Copy link
Collaborator

@sbillinge sbillinge left a comment

Choose a reason for hiding this comment

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

I think we will need a bat file for windows. you can look in regolith for example.

@bobleesj
Copy link
Collaborator Author

bobleesj commented Jan 4, 2025

I think we will need a bat file for windows. you can look in regolith for example.

I checked both labpdfproc and regolith. Labpdfproc doesnt have bat files, while regolith does. But I have noticed that regolith has scripts attached shown below while labpdfproc dispatches to 'main' function in app.py as done in this PR.

image

Could you check whether labpdfproc cli works on your windows machine? I currently dont have a windows machine with me since I am not in NY

@sbillinge
Copy link
Collaborator

I think we will need a bat file for windows. you can look in regolith for example.

I checked both labpdfproc and regolith. Labpdfproc doesnt have bat files, while regolith does. But I have noticed that regolith has scripts attached shown below while labpdfproc dispatches to 'main' function in app.py as done in this PR.

image

Could you check whether labpdfproc cli works on your windows machine? I currently dont have a windows machine with me since I am not in NY

The bat file can make the windows execution easier for windows folks. For example we can make the file clickable to start the program. We can also have it activate the env and then run, so the user doesn't have to do that. Things like that.

Copy link

codecov bot commented Jan 5, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 52.63%. Comparing base (e3e68b5) to head (c7f7b8c).
Report is 13 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #243   +/-   ##
=======================================
  Coverage   52.63%   52.63%           
=======================================
  Files           2        2           
  Lines          19       19           
=======================================
  Hits           10       10           
  Misses          9        9           

@bobleesj
Copy link
Collaborator Author

bobleesj commented Jan 5, 2025

@sbillinge ah codecov is passing - again, just re-ran CI.

The bat file can make the windows execution easier for windows folks. For example we can make the file clickable to start the program. We can also have it activate the env and then run, so the user doesn't have to do that. Things like that.

I see. For now, I made an issue. Can I address this later? #246. I am currently travelling with my macbook so it's a bit hard for me to test.

@sbillinge
Copy link
Collaborator

Can I address this later?

yes, sure.

Copy link
Collaborator

@sbillinge sbillinge left a comment

Choose a reason for hiding this comment

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

one quick question, but otherwise, this looks good to merge.

@@ -0,0 +1,3 @@
cookiecutter
black
pre-commit
Copy link
Collaborator

Choose a reason for hiding this comment

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

double-check, do we want black and pre-commit here? In general, i.e., for most packages, I think these would be optional for developers and so we wouldn't include them as being needed to use the package. However, here, since this package is for dev's to release their code, it may be appropriate to have them here......

Copy link
Collaborator Author

@bobleesj bobleesj Jan 5, 2025

Choose a reason for hiding this comment

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

@sbillinge I think so because our instructions include using black . and pre-commit as standalone cli commands, even though it's not needed to run package create.

Otherwise, we have to do

pip/conda install scikit-pacakge black pre-commit

which isn't what we want

Copy link
Collaborator Author

@bobleesj bobleesj Jan 5, 2025

Choose a reason for hiding this comment

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

I thought twice - your point is valid because our README.txt under requirement would contarct if we include pre-commit and black because it says

# build.txt should list all Conda packages required for building the package in GitHub CI,
# conda.txt should list all Conda packages required (including optional) for running the package in GitHub CI

To remain consistent, then I guess we should not list black and pre-commit.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I thought twice - your point is valid because our README.txt under requirement would contarct if we include pre-commit and black because it says

# build.txt should list all Conda packages required for building the package in GitHub CI,
# conda.txt should list all Conda packages required (including optional) for running the package in GitHub CI

To remain consistent, then I guess we should not list black and pre-commit.

yes, this feels right to me. We always expect to install a few extra tools for our dev work.

@bobleesj
Copy link
Collaborator Author

bobleesj commented Jan 5, 2025

one quick question, but otherwise, this looks good to merge.

@sbillinge replied to your comment and removed black and pre-commit

@sbillinge sbillinge merged commit df439a6 into Billingegroup:main Jan 5, 2025
5 checks passed
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.

feat: Allow create-package and update-package entry points once scikit-package is installed
2 participants