Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

added space function to art and aprint #248

Merged
merged 3 commits into from
Jul 19, 2023

Conversation

codewithnick
Copy link
Contributor

Reference Issues/PRs

#241

What does this implement/fix? Explain your changes.

added space parameter in art and aprint functions

Any other comments?

Copy link
Collaborator

@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.

Thanks for your efforts. There are some points you should take into consideration before checking my comments:

  • There is no rush. most of my comments were mentioned by @sepandhaghighi in your last PR but were not taken into action.
  • You should update CHANGELOG.md, too.
  • You should write tests for your new features.
  • You can commit on your branch and this PR will be updated. So no need for closing PR after each review and opening new one.

art/art.py Outdated Show resolved Hide resolved
art/art.py Outdated Show resolved Hide resolved
art/art.py Outdated Show resolved Hide resolved
art/art.py Outdated Show resolved Hide resolved
@codewithnick
Copy link
Contributor Author

Thanks for your efforts. There are some points you should take into consideration before checking my comments:

  • There is no rush. most of my comments were mentioned by @sepandhaghighi in your last PR but were not taken into action.
  • You should update CHANGELOG.md, too.
  • You should write tests for your new features.
  • You can commit on your branch and this PR will be updated. So no need for closing PR after each review and opening new one.

sorry about the confusion ill send a new pr, can you tell me where should i write the tests , I am confused

@sadrasabouri
Copy link
Collaborator

No need for new PR @codewithnick
Just commit and push your code to your dev branch and this PR will be updated.
For tests take a look at here.

Copy link
Collaborator

@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.

Great. Thanks for changes. I left a minor comment.

CHANGELOG.md Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jul 17, 2023

Codecov Report

Merging #248 (049cb29) into dev (0719521) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##              dev     #248   +/-   ##
=======================================
  Coverage   99.84%   99.84%           
=======================================
  Files           7        7           
  Lines        1185     1185           
  Branches       79       79           
=======================================
  Hits         1183     1183           
  Misses          1        1           
  Partials        1        1           
Impacted Files Coverage Δ
art/art.py 99.29% <100.00%> (ø)

@sadrasabouri sadrasabouri added this to the art 6.1 milestone Jul 17, 2023
@sadrasabouri
Copy link
Collaborator

I will add tests myself after your last commit. @codewithnick

Copy link
Owner

@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.

@codewithnick Well done 🔥
I comment on some minor issues.
Please address them, after that we will merge this PR 🚀

CHANGELOG.md Outdated Show resolved Hide resolved
art/art.py Outdated Show resolved Hide resolved
art/art.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@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. Since I do not have push access to @codewithnick 's fork, I'll open a PR fixing tests afterwards.

Copy link
Owner

@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.

🚀

@sepandhaghighi sepandhaghighi merged commit 292b5aa into sepandhaghighi:dev Jul 19, 2023
@codewithnick
Copy link
Contributor Author

thank you so much guys let me know if can also work on something else

@sepandhaghighi sepandhaghighi mentioned this pull request Jul 26, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants