Update frame time when renaming projects and tags #181

minchinweb · 2017-12-18T14:32:48Z

When renaming a project or a tag, update the "last_updated" time for the affected frames.

The other command in a similiar situation was edit, but it appears to already do this.

SpotlightKid · 2017-12-18T15:08:25Z

watson/cli.py

            if old_name in frame.tags:
                watson.frames[frame.id] = frame._replace(
-                    tags=[new_name if t == old_name else t for t in frame.tags]
+                    tags=[new_name if t == old_name else t for t in frame.tags],


There's a flake8 line-length error here:

Maybe we could make this a bit clearer:

new_tags = [new_name if tag == old_name else tag for tag in frame.tags] watson.frames[frame.id] = frame._replace( tags=new_tags, updated_at=updated_at )

BTW, please make sure that you have no tox failures, including in the flake8 tests, before submitting a PR.

SpotlightKid · 2017-12-18T15:12:16Z

Thanks for the PR, good catch!

I'm weary of adding more code again to cli.py for which we still don't have tests yet. I think we should put everything from line 1175 on into new methods of the Watson class (e.g. rename_project and rename_tag) and add tests.

On Windows, timestamps must (typically) be greater than 3600, otherwise they throw an `OSError` See dateutil/dateutil#197 (*arrow* depends on *dateutil*)

minchinweb · 2017-12-18T17:06:13Z

The test suite previously was spewing all sorts of errors for me, so I've fixed all the tests too (see d790bf7, the issue was dateutil + Windows + timestamps less than 3600). There remains one test that doesn't pass --> test_push, where it doesn't push any frames to the server....

I"ve refactored renaming projects and tags into the watson.py file, and added tests.

SpotlightKid

Tests pass. Good.

Test for Watson.rename_tag is missing.

the issue was dateutil + Windows + timestamps less than 3600

Can you show the errors you were getting?

If we change the time constants in the tests, we should at least add a comment explaining the reason. Better yet, we shouldn't use "magic numbers", but define them as constants at the top of the file. E.g.:

T_OFFSET = 4000

...

def test_frames_without_tags(mock, watson):
    content = json.dumps([[T_OFFSET, T_OFFSET + 10, 'foo', None]])

    mock.patch('%s.open' % builtins, mock.mock_open(read_data=content))
    assert len(watson.frames) == 1
    assert watson.frames[0].project == 'foo'
    assert watson.frames[0].start == arrow.get(T_OFFSET)
    assert watson.frames[0].stop == arrow.get(T_OFFSET + 10)
    assert watson.frames[0].tags == []

Maybe there's a way to make this change with some clever regex search & replace actions?

SpotlightKid · 2017-12-18T17:17:03Z

watson/watson.py

        return report
+
+    def rename_project(self, old_project, new_project):
+        """Takes a project and renames it in all affected frames."""


Docstring summary lines in imperative form, please.

"""Rename a project in all affected frames."""

SpotlightKid · 2017-12-18T17:17:36Z

watson/watson.py

+                           style('project', new_project)))
+
+    def rename_tag(self, old_tag, new_tag):
+        """Takes a tag and renames it in all affected frames."""


"""Rename a tag in all affected frames."""

SpotlightKid · 2017-12-18T17:23:08Z

watson/watson.py

+    def rename_project(self, old_project, new_project):
+        """Takes a project and renames it in all affected frames."""
+        if old_project not in self.projects:
+            raise click.ClickException(style(


watson.pyshouldn't really depend on click, only cli.py should. I know, it was used already in Watson.__init__, but that has been bugging me for a long time and we shouldn't add new code using it in this file. Definitely not using exceptions or functions interacting with the console from it.

In this particular case, raise a ValueError here and catch that in cli.rename.

SpotlightKid · 2017-12-18T17:23:43Z

watson/watson.py

+
+        self.frames.changed = True
+        self.save()
+        click.echo('Renamed project "{}" to "{}"'


User feedback should be in cli.py.

SpotlightKid · 2017-12-18T17:24:03Z

watson/watson.py

+    def rename_tag(self, old_tag, new_tag):
+        """Takes a tag and renames it in all affected frames."""
+        if old_tag not in self.tags:
+            raise click.ClickException(style(


Same applies as in rename_project.

SpotlightKid · 2017-12-18T17:24:22Z

watson/watson.py

+
+        self.frames.changed = True
+        self.save()
+        click.echo('Renamed tag "{}" to "{}"'


User feedback should be in cli.py.

SpotlightKid · 2017-12-18T17:26:54Z

watson/watson.py

+        self.frames.changed = True
+        self.save()
+        click.echo('Renamed project "{}" to "{}"'
+                   .format(style('project', old_project),


I'd personally break the (previous) line after .format(.

SpotlightKid · 2017-12-18T17:36:01Z

tests/test_watson.py

+# renaming project updates frame last_updated time
+def test_rename_project_with_time(mock, watson):
+    """
+    Renaming a project should update any the "last_updated" time on any


update any the "last_updated"

--->

update the "last_updated"

plus other code improvements as recommended by SpotlightKid

minchinweb · 2017-12-18T18:53:21Z

This is typical of the errors I was getting:

Moving to a T_OFFSET breaks in this case, because there is a static file frames-with-conflict that needs "offset" time values as well. watson.merge_report() would have to be refactored, as it currently expects a file.

Otherwise, the changes as suggested have been made, and a test for renaming tags has been added.

SpotlightKid · 2017-12-18T19:17:07Z

This is typical of the errors I was getting:

Are you in a UTC-8 timezone ?

This actually uncovers a problem with Watson._parse_date, which expects a unix time stamp in UTC and tries to convert it into the local timezone, which, as your example shows, fail on some systems if the UTC offset is negative. To be honest, I'm a bit astonished and shocked to realize that Watson deals with local timestamps internally, instead of UTC ones. Anyway, we'll sort this out in another issue.

minchinweb · 2017-12-19T17:23:34Z

I'm in UTC-7 (aka Mountain Standard Time).

SpotlightKid

A few further minor changes, if you don't mind...

SpotlightKid · 2017-12-19T17:35:41Z

watson/watson.py

+    def rename_project(self, old_project, new_project):
+        """Rename a project in all affected frames."""
+        if old_project not in self.projects:
+            raise ValueError


raise ValueError("Project '%s' does not exist." % old_project)

SpotlightKid · 2017-12-19T17:37:04Z

watson/watson.py

+    def rename_tag(self, old_tag, new_tag):
+        """Rename a tag in all affected frames."""
+        if old_tag not in self.tags:
+            raise ValueError


raise ValueError("Tag '%s' does not exist." % old_tag)

SpotlightKid · 2017-12-19T17:39:10Z

watson/cli.py

            raise click.ClickException(style(
                'error',
                'Tag "%s" does not exist' % old_name
            ))


except ValueError as exc: raise click.ClickException(style('error'), str(exc))

SpotlightKid · 2017-12-19T17:39:36Z

watson/cli.py

            raise click.ClickException(style(
                'error',
                'Project "%s" does not exist' % old_name
            ))


except ValueError as exc: raise click.ClickException(style('error'), str(exc))

minchinweb · 2017-12-20T15:42:46Z

Changes made as recommended, but teach me why this second form is preferred...

SpotlightKid · 2017-12-20T15:55:36Z

It's about different levels of abstraction.

If I use Watson.rename_project and it raises an exception because I passed a wrong project name, I want to get a proper error message. In cli.rename this error message is just passed on (we could use a differnt error message here, if the error message of the original exception is not user-friendly, but in this case it's fine).

Or do you mean the formatting of the error message?

minchinweb · 2017-12-20T16:56:54Z

No, you answered my questions, and it makes sense. This way, if watson.rename_project() is called from elsewhere, a useful error message is available.

willdurand requested a review from k4nar December 18, 2017 14:32

SpotlightKid reviewed Dec 18, 2017

View reviewed changes

SpotlightKid added improvement needs review needs test labels Dec 18, 2017

minchinweb added 3 commits December 18, 2017 09:33

Refactor rename from cli to Watson class

7d96020

Timestamps must be greater than 3600

d790bf7

On Windows, timestamps must (typically) be greater than 3600, otherwise they throw an `OSError` See dateutil/dateutil#197 (*arrow* depends on *dateutil*)

Add tests for renaming projects and tags

a7f1033

minchinweb force-pushed the update-time-on-rename branch from 1132130 to a7f1033 Compare December 18, 2017 17:01

SpotlightKid suggested changes Dec 18, 2017

View reviewed changes

Add test for renaming tags

eef8f2f

plus other code improvements as recommended by SpotlightKid

SpotlightKid suggested changes Dec 20, 2017

View reviewed changes

Improved error handling when renaming projects and tags

3d70b90

SpotlightKid approved these changes Dec 21, 2017

View reviewed changes

SpotlightKid merged commit 513f3c3 into jazzband:master Dec 21, 2017

minchinweb deleted the update-time-on-rename branch December 21, 2017 22:18

minchinweb mentioned this pull request Jan 3, 2018

Change aren't being synced #183

Open

Uh oh!

Update frame time when renaming projects and tags #181

Update frame time when renaming projects and tags #181

Uh oh!

Conversation

minchinweb commented Dec 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SpotlightKid commented Dec 18, 2017

Uh oh!

minchinweb commented Dec 18, 2017

Uh oh!

SpotlightKid 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!

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!

Choose a reason for hiding this comment

Uh oh!

minchinweb commented Dec 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SpotlightKid commented Dec 18, 2017

Uh oh!

minchinweb commented Dec 19, 2017

Uh oh!

SpotlightKid 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!

minchinweb commented Dec 20, 2017

Uh oh!

SpotlightKid commented Dec 20, 2017

Uh oh!

minchinweb commented Dec 20, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

minchinweb commented Dec 18, 2017 •

edited

Loading