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

Feature/cli #47

Merged
merged 20 commits into from
May 23, 2024
Merged

Feature/cli #47

merged 20 commits into from
May 23, 2024

Conversation

AHReccese
Copy link
Member

Reference Issues/PRs

#12

What does this implement/fix? Explain your changes.

  • add CLI handler
  • support requested use-cases mentioned in the associated issue's description

Any other comments?

@AHReccese AHReccese added this to the nava v0.6 milestone May 1, 2024
@AHReccese AHReccese self-assigned this May 1, 2024
@sadrasabouri sadrasabouri self-assigned this May 2, 2024
@sadrasabouri
Copy link
Member

@AHReccese please review my changes. I also invite @sepandhaghighi to review this PR.
The problem with async flag was that it causes the program closure so the sound couldn't run. The trick I used was to define a new function, play_cli, which can play sound in a while if loop is true.
AS mentioned in README.md, users can use & for playing async audio.

Copy link
Member

@sepandhaghighi sepandhaghighi left a comment

Choose a reason for hiding this comment

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

@AHReccese, @sadrasabouri

Thanks for your efforts 💯
Please take a look at my comments 🥇

README.md Outdated Show resolved Hide resolved
nava/functions.py Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
nava/__main__.py Outdated Show resolved Hide resolved
nava/__main__.py Outdated Show resolved Hide resolved
nava/__main__.py Outdated Show resolved Hide resolved
@sadrasabouri
Copy link
Member

sadrasabouri commented May 3, 2024

@sepandhaghighi can you please review again?
Requested changes are applied.

Copy link
Member Author

@AHReccese AHReccese left a comment

Choose a reason for hiding this comment

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

I reviewed your changes.
You changed everything I've written, It was better to give feedback instead of this much shared co-coding in the same zone in a single PR.
@sadrasabouri
No comment, thanks.

@AHReccese AHReccese removed their assignment May 3, 2024
@sadrasabouri
Copy link
Member

I reviewed your changes. You changed everything I've written, It was better to give feedback instead of this much shared co-coding in the same zone in a single PR. @sadrasabouri No comment, thanks.

Hi Amirhossein,
That's not true. I followed your structure for using argparser and kept the same input filenames as you added. I only added the loop and version flags to the parser and renamed the run_nava to play_cli.

Copy link
Member

@sepandhaghighi sepandhaghighi left a comment

Choose a reason for hiding this comment

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

@AHReccese Take a look at my comment 🔥

nava/__main__.py Show resolved Hide resolved
@sadrasabouri
Copy link
Member

Hey @AHReccese,
Just a friendly ⏲️

Copy link
Member

@sadrasabouri sadrasabouri left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@sepandhaghighi sepandhaghighi left a comment

Choose a reason for hiding this comment

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

LGTM 🔥

@sepandhaghighi sepandhaghighi merged commit c730633 into dev May 23, 2024
72 of 73 checks passed
@sepandhaghighi sepandhaghighi deleted the feature/CLI branch May 23, 2024 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants