Add cli for description by AnubhaAgrawal · Pull Request #259 · pimutils/todoman

AnubhaAgrawal · 2017-06-18T09:54:27Z

Solve #240 for description

WhyNotHugo

Thanks! This is great so far. Please add unit tests for these. 🙂

WhyNotHugo

Looking good so far. Please also add another test case for editing the description too.

Thanks! 🙂

AnubhaAgrawal · 2017-06-18T18:04:00Z

Please tell me that there is any problem in test_edit_description.

WhyNotHugo

Looking good. Just a few minor tweaks.

WhyNotHugo · 2017-06-18T18:30:58Z

tests/test_cli.py

+def test_edit_description(runner, todos, todo_factory):
+    todo_factory(summary='harhar\n', description='Parnidi')
+
+    result = runner.invoke(cli, ['edit', '1', '--description', 'Parnidi'])


When editing the todo, set the description to something different than the original value. Otherwise it's unclear if the edit did anything or not.

WhyNotHugo · 2017-06-18T18:31:07Z

tests/test_cli.py

+
+
+def test_edit_description(runner, todos, todo_factory):
+    todo_factory(summary='harhar\n', description='Parnidi')


No need for the \n here.

WhyNotHugo · 2017-06-18T18:58:50Z

todoman/model.py

        if priority:
            extra_where.append('AND PRIORITY > 0 AND PRIORITY <= ?')
            params.append('{}'.format(priority))
+        if description:


Oh, also, no tests seem to cover this new filter.

Sorry, I think this filter does not do anything. so, I remove it.

WhyNotHugo · 2017-06-19T20:36:51Z

todoman/cli.py

 @cli.command()
 @pass_ctx
 @click.argument('lists', nargs=-1, callback=_validate_lists_param)
+@click.option('--description',


Since you deleted the implementation for this flag, the flag should be removed too.

Note that that implementation was not wrong; just missing tests though.

I really don't understand which tests are missing.
And if I remove the flag then also it works correctly then why that implementation was not wrong.

This flag was used here. By deleting that bit of code, this flag is now unused and ignored.

My initial comment on that related bit of code was that there were no tests for it (though the code itself seemed fine). Basically, that would be a test that uses runner with 'list', '--description'.

You can either remove the flag (now unused), or undelete that code and add tests for it (I'm very much prefer the latter, of course. 🙂 ).

AnubhaAgrawal · 2017-06-21T17:34:20Z

can you please tell me where should I add these tests.( from test_model.py or test_cli.py)

…

On Tue, Jun 20, 2017 at 2:44 PM, Hugo Osvaldo Barrera < ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In todoman/cli.py <#259 (comment)>: > @@ -466,6 +468,8 @@ def move(ctx, list, ids): @cli.command() @pass_ctx @click.argument('lists', nargs=-1, callback=_validate_lists_param) ***@***.***('--description', This flag was used here <09fcd21#diff-d7b704f0d282081b24d6d106d9ea528fL656>. By deleting that bit of code, this flag is now unused and ignored. My initial comment on that related bit of code was that there were no tests for it (though the code itself seemed fine). Basically, that would be a test that uses runner with 'list', '--description'. You can either remove the flag (now unused), or undelete that code and add tests for it (I'm very much prefer the latter, of course. 🙂 ). — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#259 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AXknkfySDlmU5I95-bV5tf0Pn-kKJeFwks5sGBMZgaJpZM4N9dDs> .

-- Anubha Agrawal, Sophomore year B.tech, Computer Science Department Zakir Hussain College of Engineering & Technology, AMU

WhyNotHugo

Looking good. Just one final improvement and we're good to go.

WhyNotHugo · 2017-06-22T17:02:16Z

tests/test_cli.py

+    assert 'Kimple' in result.output
+
+
+def test_filter_description(runner, create):


Please also a second todo to this test case that is NOT included in the result (eg: one that has a descripcion that doesn't contain 'Kimple'). That way, we can make sure we're not showing todos that don't match the filtering criteria.

WhyNotHugo · 2017-06-23T14:29:40Z

tests/test_cli.py

+    )
+    result = runner.invoke(cli, ['list'])
+
+    assert 'Shubik' in result.output


Also, assert that the other todo is not in result.output. You'll want to give them different summaries to tell them apart.

AnubhaAgrawal · 2017-06-25T10:11:33Z

can you please tell me that is any other change is required??

WhyNotHugo · 2017-06-25T20:10:52Z

Looks like this comment hasn't been addressed yet.

Add cli for description

ee49842

WhyNotHugo requested changes Jun 18, 2017

View reviewed changes

WhyNotHugo reviewed Jun 18, 2017

View reviewed changes

AnubhaAgrawal added 3 commits June 18, 2017 22:29

Add test cases

3bfbf4c

Add test_edit_description

af5580d

Requested canges

09fcd21

WhyNotHugo reviewed Jun 19, 2017

View reviewed changes

Now I add test

1a604da

WhyNotHugo requested changes Jun 22, 2017

View reviewed changes

Add changes

30fe5ca

WhyNotHugo reviewed Jun 23, 2017

View reviewed changes

change Summary

b7a4a53

Base automatically changed from master to main March 9, 2021 18:59



		def test_edit_description(runner, todos, todo_factory):
		todo_factory(summary='harhar\n', description='Parnidi')

		assert 'Kimple' in result.output


		def test_filter_description(runner, create):

Conversation

AnubhaAgrawal commented Jun 18, 2017

Uh oh!

WhyNotHugo left a comment

Choose a reason for hiding this comment

Uh oh!

WhyNotHugo left a comment

Choose a reason for hiding this comment

Uh oh!

AnubhaAgrawal commented Jun 18, 2017

Uh oh!

WhyNotHugo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AnubhaAgrawal commented Jun 21, 2017 via email

Uh oh!

WhyNotHugo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AnubhaAgrawal commented Jun 25, 2017

Uh oh!

WhyNotHugo commented Jun 25, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants