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

PR: Fix saving variables as .spydata if no file extension is given #17790

Merged
merged 3 commits into from
Jul 15, 2022

Conversation

maurerle
Copy link
Contributor

@maurerle maurerle commented Apr 28, 2022

Description of Changes

While this does not fix the major file save dialog problems mentioned in #7196 it allows to save the workspace in a pickle file without finding out which extension is needed.
Currently, .mat and .spydata is supported to write data, and it makes sense to prefer .spydata here.

I also removed the filename option from the save method, as it is never used.
If the method would be called with filename='text.spydata' it would anyway save as self.filename which would be wrong.
So I hope it is fine to remove the optional parameter.

Thank you!

  • Wrote at least one-line docstrings (for any new functions)
  • Added unit test(s) covering the changes (if testable)
  • Included a screenshot or animation (if affecting the UI, see Licecap)

Issue(s) Resolved

Fixes #7196

Affirmation

By submitting this Pull Request or typing my (user)name below,
I affirm the Developer Certificate of Origin
with respect to all commits and content included in this PR,
and understand I am releasing the same under Spyder's MIT (Expat) license.

I certify the above statement is true and correct: Florian Maurer

Copy link
Member

@dalthviz dalthviz left a comment

Choose a reason for hiding this comment

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

Thank you @maurerle for helping with this! I left a comment requesting some changes (adding a comment to reference the issue and the variable to validate the extension), otherwise this LGTM 👍

As a side note, seems like the original issue post will be addressed with this PR but the root cause discussed there will not (i.e the lack of extensions when using the QFileDialog on Linux: #7196 (comment))

So, should we create a new issue to check that in the future, even maybe that could be something to do on QtPy? What do you think @ccordoba12 ?

@dalthviz dalthviz added this to the v5.3.3 milestone Jul 14, 2022
@dalthviz
Copy link
Member

Hi @maurerle if is okay for you if we add here a commit with the changes to address the minor comments left in the review to proceed with this and merge it? Let us know!

@maurerle
Copy link
Contributor Author

Ouch. I did not see your review somehow.
Sorry for the long wait.

@pep8speaks
Copy link

pep8speaks commented Jul 14, 2022

Hello @maurerle! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2022-07-14 21:29:49 UTC

@maurerle
Copy link
Contributor Author

Hi @dalthviz,
I tried to address your comment and hope to get this merged soon. Somehow I was did not notice the notification at this PR.

I also renamed the other ext variable in this file for consistency.
Hope this is okay.
Otherwise you are allowed to make edits to this PR as you wish :)

@dalthviz
Copy link
Member

dalthviz commented Jul 14, 2022

Thank you @maurerle ! Could you rebase to the latest 5.x to solve the failing tests? After that I think this should be ready

Edit: Will do a rebase to fix the tests, fix the little PEP8 issue and After the test pass then this sould be ready 👍

maurerle and others added 2 commits July 14, 2022 23:25
- removed failing filename option
- rename ext variable to more explicit extension
- rename ext in iimport_data to extension too for consistency
- add comment for related issue spyder-ide#7196
@maurerle
Copy link
Contributor Author

I rebased (which hopefully fixes the tests) and fixed the linting by looking at how other occurances of too long lines are handled.
Hope it is fine like this.

@dalthviz
Copy link
Member

Thank you so much @maurerle !

Copy link
Member

@dalthviz dalthviz left a comment

Choose a reason for hiding this comment

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

Thanks @maurerle ! LGTM 👍🏼

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

Successfully merging this pull request may close these issues.

Unable to save workspace on Linux
4 participants