Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Editor sync handling #207

Merged
merged 5 commits into from
Jun 3, 2024
Merged

Editor sync handling #207

merged 5 commits into from
Jun 3, 2024

Conversation

MarcelGeo
Copy link
Contributor

@MarcelGeo MarcelGeo commented May 30, 2024

Resolves #MerginMaps/qgis-plugin

Details

Added ➕

  • Introduce push handling for editors to improve UX
  • Introduce editor module to filter changes in push operation and checking if prevent create conflicting copy

Refactor 🔨

  • do lazy loading of metadata in method _read_metadata not in every getter
  • consider to create executor in constructor or be able to set it with method of Job

  • @wonder-sk - do we want to do rotation of logs in project directory?
  • do we want to handle only QGIS files for editor as mentioned in issue, because in this PR are checking all types of files (gpkg, qgs, mergin-config) - we can change this behavior in editor.py

@MarcelGeo MarcelGeo marked this pull request as draft May 30, 2024 08:15
@MarcelGeo MarcelGeo requested a review from wonder-sk May 30, 2024 08:15
@coveralls
Copy link

coveralls commented May 30, 2024

Pull Request Test Coverage Report for Build 9345009734

Details

  • 149 of 155 (96.13%) changed or added relevant lines in 7 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.5%) to 79.085%

Changes Missing Coverage Covered Lines Changed/Added Lines %
mergin/merginproject.py 15 17 88.24%
mergin/test/test_client.py 82 86 95.35%
Totals Coverage Status
Change from base Build 9272059448: 0.5%
Covered Lines: 3165
Relevant Lines: 4002

💛 - Coveralls

MarcelGeo added 2 commits May 31, 2024 11:03
Updates:
- added mc to PullJob class and apply_changes method
- added scripts for backup of tests performed for editor
@MarcelGeo MarcelGeo marked this pull request as ready for review May 31, 2024 09:16
Copy link
Contributor

@wonder-sk wonder-sk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks! 👏

(commented only minor stuff)

@@ -623,7 +627,7 @@ def pull_project_finalize(job):
raise ClientError("Cannot patch basefile {}! Please try syncing again.".format(basefile))

try:
conflicts = job.mp.apply_pull_changes(job.pull_changes, job.temp_dir, job.user_name)
conflicts = job.apply_changes()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to keep the direct call to job.mp.apply_pull_changes() instead of introducing a new method in PullJob (it is meant just as a structure to keep everything that is needed for a pull)

mergin/editor.py Show resolved Hide resolved
mergin/editor.py Show resolved Hide resolved
mergin/merginproject.py Outdated Show resolved Hide resolved
scripts/test_editor.py Outdated Show resolved Hide resolved
@wonder-sk
Copy link
Contributor

do we want to do rotation of logs in project directory?

probably not, it did not seem to be an issue even with projects with heavy use - and when sending logs to developers, we limit upload size (0.5 mb max if I recall correctly)

do we want to handle only QGIS files for editor as mentioned in issue, because in this PR are checking all types of files (gpkg, qgs, mergin-config)

what's in the PR looks correct to me, the issue description was not complete...

@wonder-sk wonder-sk merged commit fccd67f into master Jun 3, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issues with Editor mode
3 participants