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

Refactor: Commands execute under conda-recipe-manager namespace #29

Merged
merged 7 commits into from
Apr 11, 2024

Conversation

schuylermartin45
Copy link
Collaborator

  • Addresses feedback from Use conda-recipe-manager as the starting end point for all scripts #19
    • convert and rattler-bulk-build now execute under conda-recipe-manager
      • Alternatively, use crm for short
    • For example, instead of convert recipe/meta.yaml you now use:
      conda-recipe-manager convert recipe/meta.yaml
  • Refactors and adds missing CLI smoke tests
  • Breaks-out duplicated work in the scripts into a new commands.utils.print
    module
  • black got an update that changes spacing between from __future__ ... and
    the file comment that precedes it

@schuylermartin45 schuylermartin45 requested review from a team and removed request for a team April 10, 2024 17:47
@schuylermartin45
Copy link
Collaborator Author

build-recipe-rattler issue is resolved in #22

@schuylermartin45
Copy link
Collaborator Author

Testing if a close/open will trigger CODEOWNERS changes.

@schuylermartin45 schuylermartin45 requested a review from a team April 10, 2024 19:48
Copy link
Contributor

@kenodegard kenodegard left a comment

Choose a reason for hiding this comment

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

Almost ready to go, two minor suggestions

Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest we cherrypick this change into a separate PR.

Also since you are a member of the builds-tools team you do not need to explicitly include yourself.

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'll redact my name. I had a reason for it but now I don't remember, probably just paranoia.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As for the separate PR, why? It's a small change and I'd like to have it in sooner than later.

Copy link
Contributor

Choose a reason for hiding this comment

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

a small separate PR with such a highly focused changed like that would've already been approved and merged by now

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Lol I suppose so, maybe we can chat offline sometime. I'm sure this just comes from a difference in team experiences.

tests/parser/test_recipe_parser.py Outdated Show resolved Hide resolved
schuylermartin45 and others added 7 commits April 11, 2024 14:45
- Addresses feedback from #19
  - `convert` and `rattler-bulk-build` now execute under `conda-recipe-manager`
      - Alternatively, use `crm` for short
  - For example, instead of `convert recipe/meta.yaml` you now use:
    `conda-recipe-manager convert recipe/meta.yaml`
- Refactors and adds missing CLI smoke tests
- Breaks-out duplicated work in the scripts into a new `commands.utils.print`
  module
- `black` got an update that changes spacing between `from __future__ ...` and
  the file comment that precedes it
@schuylermartin45 schuylermartin45 force-pushed the smartin_base_cli_issue_19 branch from 62147e4 to ebc6223 Compare April 11, 2024 20:46
@schuylermartin45 schuylermartin45 merged commit e22a369 into main Apr 11, 2024
5 checks passed
@schuylermartin45 schuylermartin45 deleted the smartin_base_cli_issue_19 branch April 11, 2024 20:48
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.

2 participants