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

V2 Compatibility #74

Merged
merged 6 commits into from
Oct 5, 2022
Merged

V2 Compatibility #74

merged 6 commits into from
Oct 5, 2022

Conversation

kunztr
Copy link

@kunztr kunztr commented Jul 22, 2022

Added functionality to handle V2 businesslogic (syncing V2 files, determining when to use V2 files, ensuring old projects still work)

@kunztr kunztr added the enhancement New feature or request label Jul 22, 2022
Copy link
Member

@MattReimer MattReimer left a comment

Choose a reason for hiding this comment

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

Excellent start. just a few little nitpicks

@@ -143,7 +143,8 @@ def _syncFiles(self):
self.setProgress(self.progress)

for remote_path, remote_md5 in businesslogics.items():
local_path = os.path.join(self.business_logic_xml_dir, os.path.basename(remote_path))
# local_path = os.path.join(self.business_logic_xml_dir, os.path.basename(remote_path))
Copy link
Member

Choose a reason for hiding this comment

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

You can remove the comment here. We have git to remind us what the old value is. No sense cluttering up the codebase with old stuff.

# If this is a geopackage it's special
if new_proj_el.getparent().tag == 'Layers':
layer_name = new_proj_el.find('Path').text
if self.project.attrib['{http://www.w3.org/2001/XMLSchema-instance}noNamespaceSchemaLocation'] == 'https://xml.riverscapes.xyz/Projects/XSD/V2/RiverscapesProject.xsd':
Copy link
Member

Choose a reason for hiding this comment

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

can we set self.version on the project so that we don't need to have this logic in two places?

Also I think we should remove the hardcoded aspects of this url to the config file. I'll elaborate when we talk.

@@ -89,12 +89,15 @@ def _load_businesslogic(self):

# Case-sensitive filename we expect
bl_filename = '{}.xml'.format(self.project_type)
bl_file_dir = bl_filename
if self.project.attrib['{http://www.w3.org/2001/XMLSchema-instance}noNamespaceSchemaLocation'] == 'https://xml.riverscapes.xyz/Projects/XSD/V2/RiverscapesProject.xsd':
bl_file_dir = os.path.join('V2', bl_filename)
Copy link
Member

Choose a reason for hiding this comment

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

This variable is called dir but it has the name appended to it. Maybe rename this variable bl_full_path or something

@MattReimer
Copy link
Member

@kunztr we talked about a few ideas

use regexes and move bespoke strings up into a GLOBAL variable at the top of the file

VERSIONS = {
    "V1": "\/V1\/[a-zA-Z]+.xsd",
    "V2": "\/V2\/RiverscapesProject.xsd",
}
SCHEMALOCATION_ATTRIB = '{http://www.w3.org/2001/XMLSchema-instance}noNamespaceSchemaLocation'

# Then you can match it with regexesx
if(re.match(VERSIONS["V1"], <address here>)):
                    self.version == 'V1'

Copy link
Member

@MattReimer MattReimer left a comment

Choose a reason for hiding this comment

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

Looks great.

@MattReimer MattReimer merged commit 814b3e3 into master Oct 5, 2022
@MattReimer MattReimer deleted the v2_compatibility branch October 5, 2022 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants