Skip to content

IronPython 3 Compatibility and Update for xlrd & xlsxwriter#2422

Merged
sanzoghenzo merged 4 commits intopyrevitlabs:developfrom
trgiangv:gtv/ironpython3
Oct 20, 2024
Merged

IronPython 3 Compatibility and Update for xlrd & xlsxwriter#2422
sanzoghenzo merged 4 commits intopyrevitlabs:developfrom
trgiangv:gtv/ironpython3

Conversation

@trgiangv
Copy link
Copy Markdown
Contributor

@trgiangv trgiangv commented Oct 18, 2024

Description:

This pull request introduces:

  1. IronPython 3 Compatibility:
  • Simplify Compatible variable for handling IronPython 2/3, Cpython, netcore, netframework. trgiangv@cf51642
  • Ensured existing functionality remains stable across both versions.

  1. Updated xlrd and xlsxwriter Library:

    • Updated to the latest version.
    • Ironpython3 compatible
    • Fixed image import issues, ensuring proper embedding and display of images in Excel files.

  2. Fix build pyrevitLabs.pyRevit.Runtime only target IronPython 2.7

Additional Notes:

What challenges are we facing if we set IronPython3 as the default engine?

@jmcouffin jmcouffin added Enhancement Enhancement request [class->Improved #{number}: {title}] Python 3 Issues related to cpython engines [subsystem] IronPython Issues related to standalone IronPython installation [subsystem] Highlight Highlight these issues on Release notes dependencies Pull requests that update a dependency file labels Oct 18, 2024
@dosymep
Copy link
Copy Markdown
Member

dosymep commented Oct 18, 2024 •
edited
Loading

I think it's worth splitting the pull request into several,

  • changing the pyRevit
  • changing dependencies

I think there may be problems with dependencies and changing them, after which there will be problems with compatibility with the pyRevit code.

Also, can you resolve the merge issues?

Update:

I checked the code superficially, there is use of libraries only here, perhaps there will not be so many problems with changing versions.

pyRevit/pyrevitlib/pyrevit/interop/xl.py

Line 1 in f9fa0d2

"""Read and Write Excel Files."""

sanzoghenzo
sanzoghenzo requested changes Oct 18, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@sanzoghenzo sanzoghenzo left a comment

Choose a reason for hiding this comment

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

Nicely done @trgiangv!

Aside for the merge conflicts to resolve, I've pointed out a minor change.

Thank you for your work!

@trgiangv
Copy link
Copy Markdown
Contributor Author

trgiangv commented Oct 19, 2024 •
edited
Loading

I think it's worth splitting the pull request into several,

  • changing the pyRevit
  • changing dependencies

I think there may be problems with dependencies and changing them, after which there will be problems with compatibility with the pyRevit code.

Also, can you resolve the merge issues?

Update:

I checked the code superficially, there is use of libraries only here, perhaps there will not be so many problems with changing versions.

pyRevit/pyrevitlib/pyrevit/interop/xl.py

Line 1 in f9fa0d2

"""Read and Write Excel Files."""

Hi @dosymep

xlrd: just fix the IronPython3 compatible
XlsxWriter: I've got the problem with writing images to the Excel file, update to the latest version solve that
pyrevitLabs.pyRevit.Runtime: Currently, I have no idea to fix this, I can only identify the problem so that someone else may address it. However, rebuilding locally with IronPython3 resolved the problem for me.

sanzoghenzo
sanzoghenzo requested changes Oct 19, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@sanzoghenzo sanzoghenzo left a comment

Choose a reason for hiding this comment

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

Last round; I'm not sure if the xlrd.examples needs to be there, so you can ignore that comment

sanzoghenzo
sanzoghenzo reviewed Oct 19, 2024
View reviewed changes
@sanzoghenzo
Copy link
Copy Markdown
Contributor

sanzoghenzo commented Oct 19, 2024

Thank you very much @trgiangv !
It looks good on my side, @dosymep do you think we're good to go?

@trgiangv, if you don't mind, can you please rebase this pr and squash the commits so we have a commit for the pyrevit compat changes and another for the excel libraries update?

dosymep
dosymep approved these changes Oct 19, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@dosymep dosymep left a comment

Choose a reason for hiding this comment

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

good work 👍

@sanzoghenzo
Copy link
Copy Markdown
Contributor

sanzoghenzo commented Oct 19, 2024

Your rebase was a little messed up, I took the liberty to fix it.

I think we're good to go!

@sanzoghenzo sanzoghenzo merged commit cff2491 into pyrevitlabs:develop Oct 20, 2024
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Oct 20, 2024

📦 New work-in-progress (wip) builds are available for 5.0.0.24294+1030-wip

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Oct 21, 2024

📦 New work-in-progress (wip) builds are available for 5.0.0.24295+0909-wip

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Oct 22, 2024

📦 New public release are available for 5.0.0.24174+2300

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sanzoghenzo sanzoghenzo sanzoghenzo approved these changes

@dosymep dosymep dosymep approved these changes

@thumDer thumDer Awaiting requested review from thumDer

Assignees

No one assigned

Labels

dependencies Pull requests that update a dependency file Enhancement Enhancement request [class->Improved #{number}: {title}] Highlight Highlight these issues on Release notes IronPython Issues related to standalone IronPython installation [subsystem] Python 3 Issues related to cpython engines [subsystem]

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@trgiangv @dosymep @sanzoghenzo @jmcouffin