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

New Cleo CLI #203

Merged
merged 13 commits into from
Oct 16, 2023
Merged

New Cleo CLI #203

merged 13 commits into from
Oct 16, 2023

Conversation

JacobGinsparg
Copy link
Contributor

@JacobGinsparg JacobGinsparg commented Oct 12, 2023

Redid the CLI much in the same fashion as how Poetry has theirs set up (see here) using Cleo.

@JacobGinsparg JacobGinsparg marked this pull request as ready for review October 13, 2023 02:03
@JacobGinsparg JacobGinsparg linked an issue Oct 13, 2023 that may be closed by this pull request
]


def load_scooze_command(name: str) -> Callable[[], Command]:
Copy link
Contributor

Choose a reason for hiding this comment

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

What does the empty array [] mean in this type hint?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm reading that as "the Callable it returns takes no arguments", but would be good to confirm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep it should always be Callable[[<args type list>], <return type>]

name = "delete"
description = "Delete collections from the database."

options = [
Copy link
Contributor

Choose a reason for hiding this comment

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

Should probably use arguments instead of options so that the load and delete commands are consistent. Thoughts?

image

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed on preferring consistency; would that change impact anything else in here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Think I just autopiloted this. I'll fix it

]


def load_scooze_command(name: str) -> Callable[[], Command]:
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm reading that as "the Callable it returns takes no arguments", but would be good to confirm

name = "delete"
description = "Delete collections from the database."

options = [
Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed on preferring consistency; would that change impact anything else in here?



def delete_collection(coll: DbCollection):
clean = input(f"Delete existing {coll}? [y/n] ") in "yY"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit at my past self that it'd be convenient to fix here: can this N be capital to indicate it's the default? (and similar for the other files)


def handle(self):
# TODO(#201) - Add local MongoDB built-in support
print("Usage: `scooze setup docker` or `scooze setup local`")
Copy link
Contributor

Choose a reason for hiding this comment

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

Fine to leave this until the later ticket, but this print should indicate local isn't yet supported in CLI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Easy enough to add that here since it'd be moot when #201 gets worked on.


def handle(self):
# TODO(#201) - Add local MongoDB built-in support
print("Usage: `scooze teardown docker` or `scooze teardown local`")
Copy link
Contributor

Choose a reason for hiding this comment

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

Fine to leave this until the later ticket, but this print should indicate local isn't yet supported in CLI.

name = "delete"
description = "Delete collections from the database."

arguments = [
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fjork3 @iambroadband as this is currently written now it'll accept an arbitrary list of string args and will act accordingly if it see any of all, cards, or decks. Any other things passed in will be ignored. Do we want to print a message that other stuff passed in was ignored? It'll already say it needs valid input if none of what we expect is passed, but extra stuff isn't handled yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would make sense to print the ignored options too.

@JacobGinsparg
Copy link
Contributor Author

I changed all the prints to use the built-in Cleo Command printer self.line which lets us colorize things if we want. If you see anything you think we should colorize I can update.

def handle(self):
# TODO(#201) - Add local MongoDB built-in support
self.line(
"Usage: <fg=cyan>scooze setup docker</> or <fg=cyan>scooze setup local</>. Local support coming soon."
Copy link
Contributor

Choose a reason for hiding this comment

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

Message indicating that this isn't implemented. Same as teardown local.


def handle(self):
# TODO(#201) - Add local MongoDB built-in support
self.line(
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as local setup. Not implemented

@JacobGinsparg JacobGinsparg merged commit 24e8db6 into dev Oct 16, 2023
@JacobGinsparg JacobGinsparg deleted the cli_rework branch October 16, 2023 19:23
iambroadband added a commit that referenced this pull request Nov 10, 2023
## [1.0.5] - 2023-11-09

### Added

- Added scooze_id to CardModel and Card. Changed the MongoDB _id to scooze_id. ([#193](#193))
- Added AsyncScoozeApi as a way to use API endpoints in an async context (fixes Jupyter compatibility) ([#199](#199))
- Add Docker support for starting MongoDB via the CLI ([#200](#200))
- CLI rework to be more robust ([#203](#203))
- Github Actions to test on push and deploy on tag ([#211](#211))
- Add `cmc` field to top level for reversible cards ([#212](#212))

### Changed

- Changed the database lookup behavior to treat _id and scooze_id as the same. Also support snake case and camel case for property names. ([#205](#205))
- Added `None` as valid return type in normalizers ([#190](#190))
- Use `super().__init__()` for Card subclasses ([#217](#217))

### Fixed

- Fixed the use of mutable default arguments ([#188](#188))
- Fixed improper runner call in API init ([#190](#190))
- Fixed missing `await` call ([#190](#190))

### Docs

- More completely document possible exceptions ([#209])(#209)

---------

Co-authored-by: Ben Horkley <ben.horkley@gmail.com>
Co-authored-by: Jacob Ginsparg <jacobginsparg@gmail.com>
Co-authored-by: Clayton Mentzer <cwolfmentzer@gmail.com>
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.

Update our CLI to follow Poetry's console design pattern
3 participants