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

SaveInformation::canSave is not required #1619

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fnkkio
Copy link
Contributor

@fnkkio fnkkio commented Nov 21, 2019

In case Utils::getSavePath is called with valid fileName and directory, SaveInformation::canSave would never be set to true; moreover, this data member is not required since we can just return an empty SaveInformation::path in case we didn't get a valid save path.

@Emdek
Copy link
Member

Emdek commented Nov 22, 2019

@fnkkio, perhaps it was unfinished, what about turning it into helper method instead?

@fnkkio
Copy link
Contributor Author

fnkkio commented Nov 22, 2019

@fnkkio, perhaps it was unfinished, what about turning it into helper method instead?

I'm sorry but what do you mean about helper method ? It is already in the Utils namespace.

@Emdek
Copy link
Member

Emdek commented Nov 22, 2019

@fnkkio, helper method in SaveInformation structure, these are quite common here, usually in form of isValid() helpers.

@fnkkio
Copy link
Contributor Author

fnkkio commented Nov 22, 2019

@Emdek Added helper method.

@Emdek
Copy link
Member

Emdek commented Nov 22, 2019

@fnkkio, yeah, I meant adding it and using it. :-D

I kind of expected that you would keep the same name (canSave()) but I guess that isValid() is also fitting.

@fnkkio fnkkio force-pushed the fixes/fix_utils_getsavepath branch from dc1e2e9 to e7f602b Compare November 22, 2019 13:21
@fnkkio
Copy link
Contributor Author

fnkkio commented Nov 22, 2019

@Emdek Updated.

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