search

dave-howard / vsdx

vsdx - A python library for processing .vsdx files
BSD 3-Clause "New" or "Revised" License
70 stars 25 forks source link

Cleanup - VisioFile.page_xml_by_file_path #17

Closed stevensultana closed 3 years ago

stevensultana commented 3 years ago

From what I gathered, VisioFile.page_xml_by_file_path is not exactly being used anywhere except for VisioFile.set_page_max_id(), where a for loop can replace the functionality.

I did a quick test and removed the references to it, and things seemed to continue working as expected.

Is there a use case that I am missing? Or may I open a PR?

I think that if it is not in use, removing it will remove something that a contributor would need to keep in mind or worry about.

dave-howard commented 3 years ago

Hi stevensultana - let me look into that and I'll comment back here

dave-howard commented 3 years ago

Hi again - yes I agree - it's a legacy/duplicate from initial implementation and Page.xml and Page.filename store the same information. So that suggested PR would be an improvement.

stevensultana commented 3 years ago

Hi Dave,

I've removed page_xml_by_file_path, master_page_xml_by_file_path and also page_max_ids (and amended the necessary functions that used these attributes).

As a consequence, I moved a couple of functions that were in VisioFile to the Page and Shape classes, moving towards passing around and working with the Pages and Shapes, rather than Elements.

Now that I'm at it, shall I keep going with this? For example, the VisioFile.copy_shape() takes the page xml and page filename. Shall I migrate this to taking a Page object? This would be a change that impacts the users (although technically users should still be using Shape.copy()). What do you think? The alternative, I guess, it to add a bit of logic which counts the number of arguments, etc. Then maybe we can add a deprecation warning if users use VisioFile.copy_shape() with page.xml and page.filename. What do you think?

I'll open a PR of the current progress, and maybe we can move the discussion there.

Thanks!