analytics: refactor into a module#2826
Conversation
ghost
mentioned this pull request
ghost
changed the title
Suor
left a comment
Suor
left a comment
There was a problem hiding this comment.
Removing a class, but splitting it into several files kind of defeat the purpose of trying to make it more readable.
Agree, @Suor 🙂 |
ghost
changed the title
|
Tests were sort of exhaustive |
|
@MrOutis Tests are still failing, please take a look. |
There was a problem hiding this comment.
Looks pretty good! I'm still worried about breaking the backend analytics logic by this, so please see some requests for tests up above. Really don't fancy working on analytics backend these days, so we need to do our best to make sure that the reports are exactly the same as they were before and they won't break anything.
ghost
dismissed
efiop’s stale review
excellent observations, @efiop, thanks again for the review 👍 mind checking if my changes address your comments?
| Fixture to prevent modifying the actual global config | ||
| """ | ||
| with mock.patch( | ||
| "dvc.config.Config.get_global_config_dir", return_value=str(tmp_path) |
There was a problem hiding this comment.
removed the patch for fspath and now it doesn't work with pathlib.Path in 3.5: #2903
There was a problem hiding this comment.
It does for me, patch is only needed for Python 3.5 builtin pathlib, but pytests tmp_path uses pathlib2.
There was a problem hiding this comment.
@Suor we've been using str in all other places, so it is no biggie. We need to revisit that more carefully later and fix everywhere.
There was a problem hiding this comment.
my bad, then, @Suor , I just assumed it was using builtin pathlib
| def test_send(mock_post, tmp_path): | ||
| url = "https://analytics.dvc.org" | ||
| report = {"name": "dummy report"} | ||
| fname = str(tmp_path / "report") |
| with open(fname, "w") as fobj: | ||
| json.dump(report, fobj) |
There was a problem hiding this comment.
(tmp_path / "report").write_text(json.dumps(report))There was a problem hiding this comment.
@Suor , would need to use fspath on analytics.send:
@mock.patch("requests.post")
def test_send(mock_post, tmp_path):
url = "https://analytics.dvc.org"
report = {"name": "dummy report"}
file = (tmp_path / "report")
data = convert_to_unicode(json.dumps(report))
file.write_text(data)
analytics.send(fspath(file))
assert mock_post.called
assert mock_post.call_args.args[0] == urlI'd prefer to leave it like this for now, since we lack support for python 3.5 🙁
There was a problem hiding this comment.
Only we don't, what exactly doesn't work in Python 3.5 for you?
4 participants
Follow up from discussion on #2819
TODO: