igneus / calendarium-romanum

liturgical calendar library (Roman Catholic, post-Vatican II)
50 stars 21 forks source link

Load from filesystem #12

Closed simonszu closed 6 years ago

simonszu commented 6 years ago

Ì have added the ability to specify a sanctorale file from file system. The query command first assumes the given option string to be a filesystem path, and checks if a file exists at this location. If yes, it tries to load this file as a data file and dies if it isn't successful. If there does not exist a file at the given location, query tries to interpret the given string as the name of a predefined sanctorale file, and proceeds in the known behavior.

simonszu commented 6 years ago

I have implemented your annotations. I am not sure about squashing this commit into the last one. On the one hand the changes were very small and do not need to appear in the global commit log, on the other hand the commit message grew bigger. I didn't want to check if i can squash a commit into another commit which was not directly before the to-squash commit, so it is how it is.

igneus commented 6 years ago

No squashing necessary, I will be happy to merge as is - but there is currently one failing spec (see the link to Travis CI below).

simonszu commented 6 years ago

I see. It is strange, since rake spec runs without errors on my machine. In the output, travis says it fails on the check when querying an invalid file from the file system, it should output "Invalid file format". When manually executing this query command, i get "Invalid file format" as well.

So i really cannot understand why Travis fails, i'm sorry. Maybe you can pull this pull request on your machine and run the spec manually for yourself, so we can determine if it is Travis' fault or my machine's fault.

igneus commented 6 years ago

See the report of the failed expectation once more: the expected output is there, but it is preceded by a deprecation warning. The warning is being printed to the STDERR, so I suggest to check only standard output (exposed by aruba through method all_stdout) instead of all output.

(We should really get rid of the warning, too, but that's out of the scope of this PR.)

simonszu commented 6 years ago

Well, the die-Message is printed to STDERR as well (see https://github.com/igneus/calendarium-romanum/blob/master/lib/calendarium-romanum/cli.rb#L107). Should i change it there as well?

If not, i fear that the tests check for standard output, and fail, since the error messages are sent to STDERR, and STDOUT stays empty.

igneus commented 6 years ago

Stupid me, I didn't realize we print error output to STDERR. (And we definitely don't want to change anything about this.) Then I would change the expectation from ... start_with ... to ... include ... or ... end_with ....

simonszu commented 6 years ago

No problem. Here you go. ;)

igneus commented 6 years ago

Thanks, merging.