Merge pull request #112 from Pythagora-io/fix/save_file

Fixed issues with `Project.save_file()`

pilot/const/function_calls.py

@@ -284,7 +284,7 @@ DEV_STEPS = {
                 'properties': {
                     'files': {
                         'type': 'array',
-                        'description': f'List of files that need to be analized to implement the reqired changes.',
+                        'description': f'List of files that need to be analyzed to implement the required changes.',
                         'items': {
                             'type': 'string',
                             'description': f'A single file name that needs to be analized to implement the reqired changes. Remember, this is a file name with path relative to the project root. For example, if a file path is `{{project_root}}/models/model.py`, this value needs to be `models/model.py`.',

pilot/helpers/Project.py

@@ -1,5 +1,7 @@
 import json
-
+import os
+import re
+from typing import Tuple
 from utils.style import green_bold, yellow_bold, cyan, white_bold
 from const.common import IGNORE_FOLDERS, STEPS
 from database.database import delete_unconnected_steps_from, delete_all_app_development_data
@@ -217,20 +219,15 @@ class Project:
         """
         # TODO fix this in prompts
         if 'path' not in data:
-            data['path'] = ''
+            data['path'] = data['name']
 
-        if 'name' not in data:
-            data['name'] = ''
-
-        if ' ' in data['name'] or '.' not in data['name']:
-            if not data['path'].startswith('./') and not data['path'].startswith('/'):
-                data['path'] = './' + data['path']
-            data['name'] = data['path'].rsplit('/', 1)[1]
-
-        if '/' in data['name']:
+        if 'name' not in data or data['name'] == '':
+            data['name'] = os.path.basename(data['path'])
+        elif not data['path'].endswith(data['name']):
             if data['path'] == '':
-                data['path'] = data['name'].rsplit('/', 1)[0]
-            data['name'] = data['name'].rsplit('/', 1)[1]
+                data['path'] = data['name']
+            else:
+                data['path'] = data['path'] + '/' + data['name']
         # TODO END
 
         data['path'], data['full_path'] = self.get_full_file_path(data['path'], data['name'])
@@ -243,30 +240,31 @@ class Project:
                 update={ 'name': data['name'], 'path': data['path'], 'full_path': data['full_path'] })
             .execute())
 
-    def get_full_file_path(self, file_path, file_name):
+    def get_full_file_path(self, file_path: str, file_name: str) -> Tuple[str, str]:
         file_path = file_path.replace('./', '', 1)
-        file_path = file_path.rsplit(file_name, 1)[0]
+        file_path = os.path.dirname(file_path)
+        file_name = os.path.basename(file_name)
 
-        if file_path.endswith('/'):
-            file_path = file_path.rstrip('/')
+        paths = [file_name]
 
-        if file_name.startswith('/'):
-            file_name = file_name[1:]
+        if file_path != '':
+            paths.insert(0, file_path)
 
-        if not file_path.startswith('/') and file_path != '':
-            file_path = '/' + file_path
+        if file_path == '/':
+            absolute_path = file_path + file_name
+        else:
+            if not re.match(r'^/|~|\w+:', file_path):
+                paths.insert(0, self.root_path)
+            absolute_path = '/'.join(paths)
 
-        if file_name != '':
-            file_name = '/' + file_name
-
-        return (file_path, self.root_path + file_path + file_name)
+        return file_path, absolute_path
 
     def save_files_snapshot(self, development_step_id):
         files = get_files_content(self.root_path, ignore=IGNORE_FOLDERS)
         development_step, created = DevelopmentSteps.get_or_create(id=development_step_id)
 
         for file in files:
-            print(cyan(f'Saving file {file["path"] + "/" + file["name"]}'))
+            print(cyan(f'Saving file {(file["path"])}/{file["name"]}'))
             # TODO this can be optimized so we don't go to the db each time
             file_in_db, created = File.get_or_create(
                 app=self.app,

pilot/helpers/files.py

@@ -29,7 +29,7 @@ def get_files_content(directory, ignore=[]):
                 file_content = f.read()
 
             file_name = os.path.basename(path)
-            relative_path = path.replace(directory, '').replace('/' + file_name, '')
+            relative_path = path.replace(directory, '').replace('\\', '/').replace('/' + file_name, '')
             return_array.append({
                 'name': file_name,
                 'path': relative_path,

pilot/helpers/test_Project.py

@@ -0,0 +1,96 @@
+import pytest
+from unittest.mock import Mock, patch
+from helpers.Project import Project
+
+
+project = Project({
+        'app_id': 'test-project',
+        'name': 'TestProject',
+        'app_type': ''
+    },
+    name='TestProject',
+    architecture=[],
+    user_stories=[]
+)
+project.root_path = "/temp/gpt-pilot-test"
+project.app = 'test'
+
+
+@pytest.mark.parametrize('test_data', [
+    {'name': 'package.json', 'path': 'package.json', 'saved_to': '/temp/gpt-pilot-test/package.json'},
+    {'name': 'package.json', 'path': '', 'saved_to': '/temp/gpt-pilot-test/package.json'},
+    {'name': 'Dockerfile', 'path': None, 'saved_to': '/temp/gpt-pilot-test/Dockerfile'},
+    {'name': None, 'path': 'public/index.html', 'saved_to': '/temp/gpt-pilot-test/public/index.html'},
+    {'name': '', 'path': 'public/index.html', 'saved_to': '/temp/gpt-pilot-test/public/index.html'},
+
+    {'name': '/etc/hosts', 'path': None, 'saved_to': '/etc/hosts'},
+    {'name': '.gitconfig', 'path': '~', 'saved_to': '~/.gitconfig'},
+    {'name': '.gitconfig', 'path': '~/.gitconfig', 'saved_to': '~/.gitconfig'},
+    {'name': 'gpt-pilot.log', 'path': '/temp/gpt-pilot.log', 'saved_to': '/temp/gpt-pilot.log'},
+], ids=['name == path', 'empty path', 'None path', 'None name', 'empty name',
+        'None path absolute file', 'home path', 'home path same name', 'absolute path with name'
+])
+@patch('helpers.Project.update_file')
+@patch('helpers.Project.File.insert')
+def test_save_file(mock_file_insert, mock_update_file, test_data):
+    # Given
+    data = {'content': 'Hello World!'}
+    if test_data['name'] is not None:
+        data['name'] = test_data['name']
+    if test_data['path'] is not None:
+        data['path'] = test_data['path']
+
+    # When
+    project.save_file(data)
+
+    # Then assert that update_file with the correct path
+    expected_saved_to = test_data['saved_to']
+    mock_update_file.assert_called_once_with(expected_saved_to, 'Hello World!')
+
+    # Also assert that File.insert was called with the expected arguments
+    # expected_file_data = {'app': project.app, 'path': test_data['path'], 'name': test_data['name'],
+    #                       'full_path': expected_saved_to}
+    # mock_file_insert.assert_called_once_with(app=project.app, **expected_file_data,
+    #                                          **{'name': test_data['name'], 'path': test_data['path'],
+    #                                             'full_path': expected_saved_to})
+
+
+@pytest.mark.parametrize('file_path, file_name, expected', [
+    ('file.txt', 'file.txt', '/temp/gpt-pilot-test/file.txt'),
+    ('', 'file.txt', '/temp/gpt-pilot-test/file.txt'),
+    ('path/', 'file.txt', '/temp/gpt-pilot-test/path/file.txt'),
+    ('path/to/', 'file.txt', '/temp/gpt-pilot-test/path/to/file.txt'),
+    ('path/to/file.txt', 'file.txt', '/temp/gpt-pilot-test/path/to/file.txt'),
+    ('./path/to/file.txt', 'file.txt', '/temp/gpt-pilot-test/path/to/file.txt'),
+])
+def test_get_full_path(file_path, file_name, expected):
+    relative_path, absolute_path = project.get_full_file_path(file_path, file_name)
+
+    # Then
+    assert absolute_path == expected
+
+
+@pytest.mark.parametrize('file_path, file_name, expected', [
+    ('/file.txt', 'file.txt', '/file.txt'),
+    ('/path/to/file.txt', 'file.txt', '/path/to/file.txt'),
+    # Only passes on Windows? ('C:\\path\\to\\file.txt', 'file.txt', 'C:\\path\\to/file.txt'),
+    ('~/path/to/file.txt', 'file.txt', '~/path/to/file.txt'),
+])
+def test_get_full_path_absolute(file_path, file_name, expected):
+    relative_path, absolute_path = project.get_full_file_path(file_path, file_name)
+
+    # Then
+    assert absolute_path == expected
+
+
+# This is known to fail and should be avoided
+# def test_get_full_file_path_error():
+#     # Given
+#     file_path = 'path/to/file/'
+#     file_name = ''
+#
+#     # When
+#     full_path = project.get_full_file_path(file_path, file_name)
+#
+#     # Then
+#     assert full_path == '/temp/gpt-pilot-test/path/to/file/'

pilot/helpers/test_files.py

@@ -0,0 +1,18 @@
+import os
+from .files import get_files_content
+
+
+def test_get_files_content():
+    # Given
+    directory = os.path.dirname(__file__)
+
+    # When
+    files = get_files_content(directory, ['.pytest_cache', '__pycache__',
+                                          'agents', 'detectors', 'project_scaffold', 'story_manager'])
+
+    # Then
+    assert len(files) > 0
+    assert files[0]['path'] == ''
+    assert files[0]['full_path'].startswith(directory)
+    # TODO: could the leading / cause files being written back to the root directory?
+    assert any(file['path'] == '/exceptions' for file in files)