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

Fix export #177

Merged
merged 4 commits into from
Oct 27, 2020
Merged

Fix export #177

merged 4 commits into from
Oct 27, 2020

Conversation

gdolle
Copy link
Contributor

@gdolle gdolle commented Oct 23, 2020

This PR should solve issue #176

@cbrnr
Copy link
Owner

cbrnr commented Oct 26, 2020

Can you give an example where the current implementation gives an error? ffilter has a default value of "*", so I don't know what the problem is.

@gdolle
Copy link
Contributor Author

gdolle commented Oct 26, 2020

try to export file->export (all functions does not have a ffilter arg)

@cbrnr
Copy link
Owner

cbrnr commented Oct 26, 2020

Ah OK! Hmm, when did I break this? It seems like the export mechanism is overly complicated, I have to figure out what all this extension and ffilter stuff does and if it is really necessary.

@gdolle
Copy link
Contributor Author

gdolle commented Oct 26, 2020

I agree it could be cleaner. (I suggest just a workaround for the bug) ;)

@cbrnr
Copy link
Owner

cbrnr commented Oct 26, 2020

Yes, at some point the exporting functionality should be refactored, but for now we need a workaround. Would it be simpler to just add the ffilter parameter to the functions in model.py that only have an fname parameter?

@gdolle
Copy link
Contributor Author

gdolle commented Oct 26, 2020

I'm not sure it is better in terms of design.
If more arguments are required later, that mean you have to change the prototype for all your export functions (with unused args)

@cbrnr
Copy link
Owner

cbrnr commented Oct 26, 2020

I've created #178 to discuss this.

mnelab/mainwindow.py Outdated Show resolved Hide resolved
@cbrnr cbrnr changed the title Patch export fnc call Fix export Oct 27, 2020
@cbrnr cbrnr merged commit 62b6a34 into cbrnr:master Oct 27, 2020
@cbrnr
Copy link
Owner

cbrnr commented Oct 27, 2020

Thanks @gdolle!

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.

2 participants