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

Protect input files from being over-written #116 #133

Merged
merged 4 commits into from
Feb 4, 2017
Merged

Protect input files from being over-written #116 #133

merged 4 commits into from
Feb 4, 2017

Conversation

gerdreiss
Copy link
Contributor

@gerdreiss gerdreiss commented Feb 3, 2017

Hi @hrj , that's the fix for the issue #116. I'm checking for potential overwriting of input files right after the program has parsed all of them, e.g. also those that are included in other input files via the "include" instruction. Please let me know if that's what you had in mind.

Edit:
Closes #116

@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 70.361% when pulling bdcabca on gerdreiss:protect-input-files into 76c80db on hrj:master.

Copy link
Owner

@hrj hrj left a comment

Choose a reason for hiding this comment

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

I liked this approach of checking before hand! Also, thanks for adding test-cases.


val inputFiles = filenamesToPaths(inputs.toSeq)
val repOutFiles = filenamesToPaths(settings.reports.flatMap(rep => filenamesToConfRelPaths(rep.outFiles)))
val expOutFiles = filenamesToPaths(settings.exports.flatMap(exp => filenamesToConfRelPaths(exp.outFiles)))
Copy link
Owner

Choose a reason for hiding this comment

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

It would be nice to move the logic of getting output paths into the Settings class. That way, if we have more types of output files tomorrow, we can remember to include them when the settings class is modified. Also, peculiarities, such as "-" files, can be confined to the Settings class.

Copy link
Contributor Author

@gerdreiss gerdreiss Feb 4, 2017

Choose a reason for hiding this comment

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

You're right, it makes sense to have that part in the Settings class.
It seems to me that this part (App.scala, line 10) could be moved to the Settings class, too:

val writesToScreen = outFiles.contains("-") || outFiles.isEmpty

What do you think?

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, that makes sense too 👍

exports += {
type = balance
format = xml
outFiles = [o01-include.ledger]
Copy link
Owner

Choose a reason for hiding this comment

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

I think these tests should be split. Otherwise, the error from "overwriting o01.ledger" will mask the absence of other errors.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 70.562% when pulling 3a901f5 on gerdreiss:protect-input-files into 76c80db on hrj:master.

@hrj hrj merged commit fa6d52d into hrj:master Feb 4, 2017
@hrj hrj added this to the Next release milestone Feb 4, 2017
@gerdreiss gerdreiss deleted the protect-input-files branch February 4, 2017 16:34
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.

Protect input files from being over-written
3 participants