Custom code directory #179

ZanderBrown · 2016-10-29T14:12:19Z

I'm not especially expecting this to be merged and I've probably upset Travis but this should help out people like @rpiuser in #178

Basically reads python_dir in settings.json to use as the save directory

Bring things upto date

ntoll · 2016-10-29T14:47:10Z

This looks useful! I'd like to merge this and release:

./mu/logic.py:117:1: E302 expected 2 blank lines, found 1
./mu/logic.py:128:30: E126 continuation line over-indented for hanging indent
./mu/logic.py:131:1: E302 expected 2 blank lines, found 1

ntoll · 2016-10-29T17:10:58Z

Also, I notice that it breaks the unit tests. 😭

Care to fix this up? It looks a useful feature for advanced users.

ZanderBrown · 2016-10-29T21:59:34Z

I'll fix it up tomorrow sorry I didn't see you reply earlier!

ZanderBrown · 2016-10-30T12:23:58Z

Anyone with a better understanding of the testing system have any pointers here?

- seems to make the tests a little happier - Actually add me to AUTHORS

ntoll · 2016-10-30T12:29:34Z

@ZanderBrown Pointers in what sense..?

ZanderBrown · 2016-10-30T12:34:06Z

What I've broken would be great :-)

I've cleared up most the errors now but the last 4 have me stuck,

For example as far as i can see I've done nothing to upset this.

>       assert mock_open.call_count == 1
E       assert 2 == 1
E        +  where 2 = <MagicMock id='1917013104'>.call_count

tests/test_logic.py:1130: AssertionError

ntoll · 2016-10-30T12:41:05Z

@ZanderBrown OK... I can't look right now but will get to it by the end of this afternoon. I'd like to see this released ASAP.

ZanderBrown · 2016-10-30T15:10:13Z

Ah i may have cracked it. mock_open.call_count counts how many files have been opened. As a result of my changes settings.json is actually opened twice during these tests

Silly thing really.

ZanderBrown · 2016-10-30T15:20:23Z

Assuming Travis likes that this should be good to merge

Summery:

Add python_dir key to settings.json
Update test_logic.py to allow for the changes in logic
Add me to AUTHORS

Notes

No tests for this feature specifically (yet?)

ZanderBrown · 2016-10-30T15:26:18Z

tests/test_logic.py

@@ -905,9 +904,10 @@ def test_save_no_path():
    ed = mu.logic.Editor(view)
    with mock.patch('builtins.open', mock_open):
        ed.save()
-    mock_open.assert_called_once_with('foo.py', 'w', newline='')
+    assert mock_open.call_count == 2


to explain this change:
before only one file would be opened that being foo.py for writing therefore assert_called_once_with worked file however getting the folder for that file to be saved into now requires opening settings.json which of course means 2 file operations hence the change in this test.

ZanderBrown · 2016-10-30T15:30:37Z

Odd the tests run fine on my machine.

carlosperate

I think we should make this a setting that is automatically saved on the json file with the rest, rather than having it as an "optional setting" that has to be manually inputted (with the default path as a fallback). If the settings file already has a value that is easy to recognise it increases is discoverability, and it also reduces the chances of the user typing incorrect json if all they have to do is replace a path.

If there is some urgency to release this (as it blocks users from using the application) then we can always look into that in a different PR.

carlosperate · 2016-10-30T16:17:40Z

About the error, I'm on a train now, so I could only have a look later tonight, but if it fails on the server but not on your computer make sure you check the paths are correct for all operating systems, as there are going to be diffencies there (test currently only run on macOS).

ntoll · 2016-10-30T16:21:21Z

@ZanderBrown use make check as the build chain does. It checks more than just the tests (pep8, flake etc).

Here's what Travis says:

E               FileNotFoundError: [Errno 2] No such file or directory: '/Users/travis/Library/Application Support/mu/settings.json'

This looks to me like your tests are checking state of the file system. The tests should run in isolation so the set up and tear down for each individual tests needs. Also, the tests shouldn't interfere with nor depend upon the user's own settings files.

Does this make sense?

ZanderBrown · 2016-10-30T16:29:17Z

Yes it does make sense although i'm getting errors for tests i directly haven't edited!

I've had a realisation though and switched

        if not os.path.exists(get_python_dir()):
            logger.debug('Creating directory: {}'.format(get_python_dir()))
            os.makedirs(get_python_dir())
        if not os.path.exists(DATA_DIR):
            logger.debug('Creating directory: {}'.format(DATA_DIR))
            os.makedirs(DATA_DIR)

for

        if not os.path.exists(DATA_DIR):
            logger.debug('Creating directory: {}'.format(DATA_DIR))
            os.makedirs(DATA_DIR)
        if not os.path.exists(get_python_dir()):
            logger.debug('Creating directory: {}'.format(get_python_dir()))
            os.makedirs(get_python_dir())

On the basis get_python_dir() reads from DATA_DIR so it must exist first

This is getting quite annoying but of course highlights why these tests exist

EDIT
Clearly got something right. Only 1 Error.

The failure is in get_settings_path() and occurs because DATA_DIR isn't actually made during the test_editor_init test as the command that would create it is a dummy to test that it was run.

ZanderBrown · 2016-10-30T17:01:00Z

Yay green tick!

I seem to have sorted all the issues raised by the great Travis

carlosperate

Thanks for spending the time on this Zander, it'll be great to finally have this feature in Mu after all the discussions!

Could we also add unit test for get_python_dir() and ensure we also have a test with settings file that contains the new key?

Btw, you can completely ignored my previous comment about not saving the key on the settings path, I briefly looked at the code on mobile and must have missed the entry on quit().

carlosperate · 2016-10-30T17:55:11Z

mu/logic.py

-#  home directory, and visible (so not a . directory)
-PYTHON_DIRECTORY = os.path.join(HOME_DIRECTORY, 'mu_code')
+# Name of the directory within the home folder to use by default
+DIRECTORY_NAME = 'mu_code'


DIRECTORY_NAME is a really generic name, could we change it to something a bit more specific?

suggestions?

MU_CODE_DIRECTORY?

carlosperate · 2016-10-30T18:06:58Z

mu/logic.py

+                if 'python_dir' in sets:
+                    # They have set a custom folder return that
+                    return sets['python_dir']
+                else:


This else statement is not really necessary, as it will just return at the end of the function.

carlosperate · 2016-10-30T18:27:55Z

mu/logic.py

@@ -631,7 +663,8 @@ def quit(self, *args, **kwargs):
                paths.append(widget.path)
        session = {
            'theme': self.theme,
-            'paths': paths
+            'paths': paths,
+            'python_dir': get_python_dir()


Could we use a different name for this? if I am reading the settings file and I see "python_dir" I would assume it would need to point to the python interpreter.
If we do change this, then I guess get_python_dir() should also be renamed as well.

i choose it as the constant being replaced was PYTHON_DIRECTORY would 'workspace' (& get_workspace()) work better?

Yeah, I like workspace, in which case my previous comment for MU_CODE_DIRECTORY should probably be WORKSPACE_DIRECTORY.

carlosperate · 2016-10-30T18:31:54Z

mu/logic.py

+                # Load up the JSON
+                sets = json.load(f)
+                if 'python_dir' in sets:
+                    # They have set a custom folder return that


Sorry about being pedantic about a comment, but technically since we are writing python_dir into the settings file then finding this key would be expected in most cases, so it doesn't really indicate a custom path has been set.

very true. it should exist from the first run onwards

ZanderBrown · 2016-10-30T19:28:26Z

Any suggestions for tests / how they would work?

ZanderBrown · 2016-10-30T19:31:31Z

mu/logic.py

    return settings_dir


+def get_workspace_dir():


Admittedly tests for this would be good.
Any suggestions of what & how?

Briefly (I have dad duty things taking up my time this evening, will reply more fully tomorrow AM), yes: ensure you set up the test with a settings file somewhere temporary and Mock away get_settings_path to return a path to this new temp settings file. Then test to your hearts' content. Does this make sense..?

makes sense. unfortunately i may not be able to work on this again until tomorrow PM

ZanderBrown · 2016-10-31T20:55:29Z

That was one nail bitingly long Travis build 😄

carlosperate · 2016-10-31T21:00:35Z

tests/test_logic.py

+    """
+    should_be = os.path.join(mu.logic.HOME_DIRECTORY,
+                             mu.logic.WORKSPACE_NAME)
+    assert mu.logic.get_workspace_dir() == should_be


Keep in mind that as it is, executing get_workspace_dir() will execute get_settings_path() which will create the settings.json file on the default location. Even if we don't care about creating this file, if a different test has also done this (in which case it should probably be changed), then we might be reading settings data that might be different than expected.
So we should probably mock get_settings_path and the file open to read an empty json object.

actually it's attempts to create a file where blocked by Travis throwing a FileNotFoundError which i added a try ... except for

We should not relay on the OS to force the code path to test a specific feature. If I have opened Mu at least once in my computer and then ran this test it will never test the last return (with the default value) from get_workspace_dir(). Also, if I have modified my settings.json file to point to a different workspace then this test will fail.

okay changes incoming

ZanderBrown · 2016-11-02T20:48:43Z

Anything else or is this ready now?

ntoll · 2016-11-03T09:08:13Z

I'm at "real work" right now but hope to be able to spend some time just before tonight's @ldnpydojo to get this merged in. :-)

ntoll · 2016-11-06T11:08:58Z

I don't get 100% test coverage. :-(

Don't worry, I've coded up a test. :-)

I'll check back later to ensure the builds have succeeded against new master and will update with a new release.

ZanderBrown and others added 2 commits October 29, 2016 14:28

['python_dir'] key in settings

d283703

Merge pull request #1 from ZanderBrown/libify

2ef58b4

Bring things upto date

ZanderBrown mentioned this pull request Oct 29, 2016

Better compatibility for restricted environments in schools #178

Closed

ZanderBrown added 2 commits October 30, 2016 11:34

(hopefully) make travis happy

6af4dd8

add some comments, add me to AUTHORS

10c000f

Extra error handling

2b89b90

- seems to make the tests a little happier - Actually add me to AUTHORS

cracked it.

251b1c3

Silly thing really.

ZanderBrown commented Oct 30, 2016

View reviewed changes

carlosperate reviewed Oct 30, 2016

View reviewed changes

OK so lets create the directories the other way round

3ceac85

catch errors in creating the settings file

62d6929

carlosperate reviewed Oct 30, 2016

View reviewed changes

rename get_python_dir to get_workspace_dir

0f05aba

ZanderBrown commented Oct 30, 2016

View reviewed changes

ZanderBrown mentioned this pull request Oct 30, 2016

On raspbian, clicking the Files panel adds a new panel every time #181

Closed

ZanderBrown added 2 commits October 31, 2016 19:31

test_get_workspace() and check for WORKSPACE_NAME

085d165

silly 6 characters slipped through the net

e53a946

carlosperate reviewed Oct 31, 2016

View reviewed changes

use a second sample settings.json for relaiable tests

23dd497

ntoll merged commit 23dd497 into mu-editor:master Nov 6, 2016

ZanderBrown mentioned this pull request Dec 25, 2016

Default folder on Windows not in Documents #86

Closed

Custom code directory #179

Custom code directory #179

Conversation

ZanderBrown commented Oct 29, 2016

ntoll commented Oct 29, 2016

ntoll commented Oct 29, 2016

ZanderBrown commented Oct 29, 2016

ZanderBrown commented Oct 30, 2016

ntoll commented Oct 30, 2016

ZanderBrown commented Oct 30, 2016

ntoll commented Oct 30, 2016

ZanderBrown commented Oct 30, 2016

ZanderBrown commented Oct 30, 2016

Choose a reason for hiding this comment

ZanderBrown commented Oct 30, 2016 • edited Loading

carlosperate left a comment

Choose a reason for hiding this comment

carlosperate commented Oct 30, 2016

ntoll commented Oct 30, 2016

ZanderBrown commented Oct 30, 2016 • edited Loading

ZanderBrown commented Oct 30, 2016

carlosperate left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZanderBrown Oct 30, 2016 • edited Loading

Choose a reason for hiding this comment

ZanderBrown commented Oct 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZanderBrown Oct 30, 2016 • edited Loading

Choose a reason for hiding this comment

ZanderBrown commented Oct 31, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlosperate Oct 31, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZanderBrown commented Nov 2, 2016

ntoll commented Nov 3, 2016

ntoll commented Nov 6, 2016

ZanderBrown commented Oct 30, 2016 •

edited

Loading

ZanderBrown commented Oct 30, 2016 •

edited

Loading

carlosperate left a comment •

edited

Loading

ZanderBrown Oct 30, 2016 •

edited

Loading

ZanderBrown Oct 30, 2016 •

edited

Loading

carlosperate Oct 31, 2016 •

edited

Loading