Bugfix/issue 2727 #2740

dzekem · 2020-10-27T21:22:29Z

No description provided.

pep8speaks · 2020-10-27T21:22:46Z

Hello @dzekem! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

In the file mycroft/client/enclosure/mark1/__init__.py:

Line 136:13: E128 continuation line under-indented for visual indent

In the file mycroft/lock/__init__.py:

Line 102:44: W291 trailing whitespace

In the file mycroft/util/audio_test.py:

Line 76:80: E501 line too long (82 > 79 characters)
Line 77:80: E501 line too long (89 > 79 characters)

In the file mycroft/util/file_utils.py:

Line 86:80: E501 line too long (84 > 79 characters)

Comment last updated at 2020-11-18 15:57:29 UTC

devops-mycroft · 2020-10-27T21:23:05Z

Hello, @dzekem, thank you for helping with the Mycroft project! We welcome everyone
into the community and greatly appreciate your help as we work to build an AI
for Everyone.

To protect yourself, the project, and users of Mycroft technologies we require
a Contributor Licensing Agreement (CLA) before accepting any code
contribution. This agreement makes it crystal clear that along with your
code you are offering a license to use it within the confines of this project.
You retain ownership of the code, this is just a license.

Please visit https://mycroft.ai/cla to initiate this one-time signing. Thank
you!

devops-mycroft · 2020-10-27T22:12:38Z

Voight Kampff Integration Test Failed (Results)

krisgesling · 2020-10-28T02:34:51Z

Hi Dzekem, thanks for flagging this and creating a PR to fix it!

I'm wondering if it's worth us adding a small helper function to mycroft.util, something like create_temp_path() that takes in 1 or more strings, returning these as a single path inside tempfile.gettempdir()?

Maybe something like:

import os
import tempfile 

def create_temp_path(*args):
    try:
        path = os.path.join(tempfile.gettempdir(), *args)
    except TypeError:
        path = None
	LOG.error('Could not create a temp path, create_temp_path() only accepts Strings')
    return path

This would mean we can use it across Mycroft in a more simplified fashion:

from mycroft.util import create_temp_path

temp_directory = create_temp_path('mycroft')
temp_file = create_temp_path('somefile.ext')

I haven't seen a CLA request come through so please let me know if you run into any problems or have questions about it.

I also saw the Github weirdness on this PR, it's really easy to do and happens to all of us. I'd suggest rebasing your PR off the dev branch. There's instructions for this in our contributing guidelines but again shout out if you run into problems.

For the PEP8 issues I'd suggest using an automated tool like autopep8 to check and fix these. They all seem like tiny issues, but enforcing this helps us keep the codebase smaller, cleaner and more consistent which makes it easier to read.

If you aren't already feel free to join us in Mycroft Chat, the dev channel is a good place to ask questions on all of the above.

Thanks

devops-mycroft · 2020-10-29T16:03:23Z

Voight Kampff Integration Test Failed (Results)

forslund

Generally looks great, thanks for fixing these issues!

I had a couple of nit-picks to keep the coding style in line with the current file. The big issues I saw was a changed path and a small typo in a function call

forslund · 2020-10-30T06:58:46Z

mycroft/client/enclosure/mark1/__init__.py

            mixer.setvolume(prev_vol)
-            play_wav("/tmp/test.wav").communicate()
+            play_wavv(os.path.join(tempfile.gettempdir(), "test.wav").communicate()


typo, should be play_wav

forslund · 2020-10-30T06:59:32Z

mycroft/client/enclosure/mark1/__init__.py

-            new_config = {'opt_in': enable}
-            user_config = LocalConf(USER_CONFIG)
+            new_config={'opt_in': enable}
+            user_config=LocalConf(USER_CONFIG)


spaces around = signs. Here and a lot of places

something is confusing autopep8 in this file, it intentionally removes the white spaces here...
I've just spent far too long messing with my config but it only seems to happen for this file.

Going to resort to manually hitting the space bar 🤯

and the problem was OBVIOUSLY (read - not obviously) a missing closing parentheses on line 134 🤦

also apologies to @dzekem when I peeked into this PR I thought you hadn't responded to the change requests, but I see now that you ran autopep8 on everything it was just masked by this obscure typo!

forslund · 2020-10-30T07:00:39Z

mycroft/lock/__init__.py

@@ -98,7 +99,8 @@ class Lock:  # python 3+ 'class Lock'

    #
    # Class constants
-    DIRECTORY = '/tmp/mycroft'
+    DIRECTORY = os.path.join(tempfile.gettempdir(),
+                             "mycroft", "ipc")  # '/tmp/mycroft'


This seems to change the directory from /tmp/mycroft to /tmp/mycroft/ipc

forslund · 2020-10-30T07:01:51Z

mycroft/skills/skill_updater.py

 import sys
 from datetime import datetime
 from time import time
-


This space should remain here I think since above are builtin packages and msm is a non-system package but not included in the mycroft module

forslund · 2020-10-30T07:03:38Z

test/unittests/lock/test_lock.py

-        if exists('/tmp/mycroft'):
-            rmtree('/tmp/mycroft')
+        if exists(os.path.join(tempfile.gettempdir(), "mycroft")):
+            rmtree(os.path.join(tempfile.gettempdir(), "mycroft"))


use ' like the rest of the file to keep it consistent. (here and other places in this file)

forslund · 2020-10-30T07:08:31Z

@krisgesling we do have a similar function in the utils ensure_directory_exists() which will create a directory if missing.

forslund · 2020-10-30T14:35:19Z

mycroft/util/audio_test.py

-        '-f', '--filename', dest='filename', default="/tmp/test.wav",
-        help="Filename for saved audio (Default: /tmp/test.wav)")
+        '-f', '--filename', dest='filename', default=os.path.join(tempfile.gettempdir(), "test.wav"),
+        help="Filename for saved audio (Default:  os.path.join(tempfile.gettempdir(), "test.wav")")


here you should probably do
Saw this flagged by travis, it should probably be

help="Filename for saved audio (Default: {}".format(os.path.join(tempfile.gettempdir(), "test.wav")))

devops-mycroft · 2020-11-03T19:46:44Z

Voight Kampff Integration Test Failed (Results)

devops-mycroft · 2020-11-03T20:06:28Z

Voight Kampff Integration Test Failed (Results)

forslund

Seems to be a couple of minor issues still. But we're getting there :)

forslund · 2020-11-03T21:59:22Z

mycroft/client/enclosure/mark1/__init__.py

 import sys
+import os
+import tempfile


imported twice now?

forslund · 2020-11-03T22:00:22Z

mycroft/client/enclosure/mark1/__init__.py

@@ -220,7 +223,7 @@ class EnclosureWriter(Thread):
    Note: A command has to end with a line break
    """

-    def __init__(self, serial, bus, size=16):
+    def __init__(self, serial, bus, size = 16):


In function-calls there should be no spaces around =.

It's weird but that's the pep8 style :/

forslund · 2020-11-03T22:02:21Z

mycroft/client/enclosure/mark1/__init__.py

            mixer.setvolume(prev_vol)
-            play_wav("/tmp/test.wav").communicate()
+            play_wav(os.path.join(tempfile.gettempdir(), 'test.wav').communicate()


I think this line is missing a closing parenthesis )

devops-mycroft · 2020-11-04T19:23:03Z

Voight Kampff Integration Test Failed (Results)

forslund · 2020-11-04T20:19:56Z

mycroft/client/enclosure/mark1/__init__.py

@@ -135,7 +134,7 @@ def process(self, data):
            time.sleep(0.5)  # Prevents recording the loud button press
            record(os.path.join(tempfile.gettempdir(), 'test.wav'), 3.0)
            mixer.setvolume(prev_vol)
-            play_wav(os.path.join(tempfile.gettempdir(), 'test.wav').communicate()
+            play_wav(os.path.join(tempfile.gettempdir(), 'test.wav').communicate())


Not sure about this but I think it should be

play_wav(os.path.join(tempfile.gettempdir(), 'test.wav')).communicate()

since play_wav returns a subprocess object we want to communicate with to ensure it's finished.

devops-mycroft · 2020-11-08T21:11:06Z

Voight Kampff Integration Test Succeeded (Results)

devops-mycroft · 2020-11-08T21:58:39Z

Voight Kampff Integration Test Succeeded (Results)

devops-mycroft · 2020-11-10T19:24:40Z

Voight Kampff Integration Test Failed (Results)

devops-mycroft · 2020-11-11T19:02:20Z

Voight Kampff Integration Test Failed (Results)

forslund

Found two small typos in the PR.

forslund · 2020-11-11T22:08:32Z

mycroft/client/enclosure/mark1/__init__.py

            mixer.setvolume(prev_vol)
-            play_wav("/tmp/test.wav").communicate()
+            play_wav(creat_temp_path('test.wav')).communicate()


should be create_temp_path

forslund · 2020-11-11T22:09:50Z

test/unittests/util/test_download.py

 from threading import Event
 from unittest import TestCase, mock

 from mycroft.util.download import (download, _running_downloads,
                                   _get_download_tmp)
+from mycroft.util import create_temp_path


should be from mycroft.util.file_utils import create_temp_path

devops-mycroft · 2020-11-12T16:30:19Z

Voight Kampff Integration Test Succeeded (Results)

dzekem · 2020-11-12T16:46:37Z

@krisgesling @forslund

I've sent a CLA request and it's been days no reply yet.

Also thank you for the suggestion of using a function which I have made use of where necessary in the code

krisgesling · 2020-11-12T20:48:13Z

Hey sorry about that, there's still some manual steps in our CLA process and I missed the email.

You should now have an email from HelloSign. Let me know if it doesn't show up or you have any questions about it :)

dzekem · 2020-11-12T21:01:29Z

Thank you I've signed and submitted it.

devops-mycroft · 2020-11-17T16:45:42Z

Voight Kampff Integration Test Failed (Results)

devops-mycroft · 2020-11-18T16:47:34Z

Voight Kampff Integration Test Failed (Results)

PureTryOut · 2021-06-10T14:21:06Z

Can't this be closed now #2892 is merged?

krisgesling · 2021-06-10T21:19:44Z

Indeed - thanks :)

devops-mycroft added the CLA: Needed Need signed CLA from https://mycroft.ai/cla label Oct 27, 2020

krisgesling added the hacktoberfest-accepted label Oct 28, 2020

krisgesling added the Status: Work in progress PR being actively worked on, not yet ready for review. label Oct 28, 2020

forslund suggested changes Oct 30, 2020

View reviewed changes

forslund reviewed Oct 30, 2020

View reviewed changes

krisgesling linked an issue Oct 30, 2020 that may be closed by this pull request

Replacing hard coded /tmp/ for some instances in the code #2727

Closed

forslund suggested changes Nov 3, 2020

View reviewed changes

forslund reviewed Nov 4, 2020

View reviewed changes

dzekem force-pushed the bugfix/issue-2727 branch from 862cdce to 9c2c93c Compare November 8, 2020 20:22

dzekem added 6 commits November 8, 2020 12:36

replaced hard coded /tmp

c3ef31e

Issue-2727 - fixing hard coded /tmp

5b99206

rectified pep8 issues

cd36260

spaces added and uniformity in code

efdb257

more fixes

3b05821

minor fixes

d560fa2

ryanjyoder force-pushed the bugfix/issue-2727 branch from 9c2c93c to d560fa2 Compare November 8, 2020 20:36

created temp path function

f120518

ran autopep8

50fe34b

forslund suggested changes Nov 11, 2020

View reviewed changes

minor errors fixed

eb2539a

pep8 error rectified

14b575a

devs-mycroft added CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) and removed CLA: Needed Need signed CLA from https://mycroft.ai/cla labels Nov 18, 2020

newline added

3db9cca

krisgesling mentioned this pull request Apr 30, 2021

Replace hardcoded use of /tmp directory #2892

Merged

1 task

krisgesling closed this Jun 10, 2021

Bugfix/issue 2727 #2740

Bugfix/issue 2727 #2740

Uh oh!

Conversation

dzekem commented Oct 27, 2020

Uh oh!

pep8speaks commented Oct 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated at 2020-11-18 15:57:29 UTC

Uh oh!

devops-mycroft commented Oct 27, 2020

Uh oh!

devops-mycroft commented Oct 27, 2020

Uh oh!

krisgesling commented Oct 28, 2020

Uh oh!

devops-mycroft commented Oct 29, 2020

Uh oh!

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

forslund commented Oct 30, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devops-mycroft commented Nov 3, 2020

Uh oh!

devops-mycroft commented Nov 3, 2020

Uh oh!

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

devops-mycroft commented Nov 4, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devops-mycroft commented Nov 8, 2020

Uh oh!

devops-mycroft commented Nov 8, 2020

Uh oh!

devops-mycroft commented Nov 10, 2020

Uh oh!

devops-mycroft commented Nov 11, 2020

Uh oh!

forslund left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

devops-mycroft commented Nov 12, 2020

Uh oh!

dzekem commented Nov 12, 2020

Uh oh!

krisgesling commented Nov 12, 2020

Uh oh!

dzekem commented Nov 12, 2020

Uh oh!

pep8speaks commented Oct 27, 2020 •

edited

Loading

forslund left a comment •

edited

Loading