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.

Sign up for GitHub

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

Optparse and Console output #3

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Optparse and Console output #3

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6847ad2
Added optparse; options -o and -y
Nov 20, 2018
a51c8a9
Console output finish added
Nov 20, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions lib/ordodo.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
require 'date'
require 'delegate'
require 'tilt'
require 'optparse'

require 'cells'
require 'cells-erb'
Expand Down
48 changes: 45 additions & 3 deletions lib/ordodo/cli.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,56 @@
module Ordodo
class CLI
def self.run!(argv)

options = {}
optparse = OptionParser.new do|opts|
# Set a banner, displayed at the top
# of the help screen.
opts.banner = "Usage: ordodo [OPTIONS] myconfig.xml"

# Define the options, and what they do
options[:outputfile] = nil
opts.on( '-o', '--output FILE', 'Write output to FILE' ) do|file|
options[:outputfile] = file
end

options[:year] = nil
opts.on( '-y', '--year YEAR', 'Specify the YEAR for which the ordo should be produced' ) do|year|
Copy link
Owner

Choose a reason for hiding this comment

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

Currently the user is expected to specify year on every invocation. This change makes year optional and sensible default (upcoming year) is used if year is not specified.

I'm not completely convinced we want this.

I see an argument in favour of this change: "Any user input, for which a sensible default can be used, should be optional." And there is such a sensible default: we can expect that a typical user would use ordodo to build calendar for the upcoming year.

On the other hand, I see this particular argument as "critical". Whenever someone (in a real-life non-development scenario) uses ordodo to generate a calendar, s/he definitely wants to generate it for some particular year. Noone ever wants "some calendar, I don't really care for the year". And in such a case it makes sense to make such an argument required.

I must admit I'm undecided about this, so unless some convincing argument is added to one side or the other, I'll just merge it as is.

Copy link
Author

Choose a reason for hiding this comment

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

That's not true. The default usage is (like written in the README) without year specification. The code works exactly as before: If the year is not specified, the "upcoming year" will be used.

I think this behaviour is reasonable. In a real life situation you want to compile the calendar for the next year (in 90% of all cases).

Copy link
Owner

Choose a reason for hiding this comment

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

You are right. I not only failed to understand my own code, but also to read my own README 😊

options[:year] = Integer(year)
Copy link
Owner

Choose a reason for hiding this comment

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

More idiomatic Ruby way of "type-casting" is by calling the subject's method: year.to_i

end

# This displays the help screen
opts.on( '-h', '--help', 'Display this screen' ) do
puts opts
exit
end
end
# Parse the command-line.
optparse.parse!

config_path = argv[0] || die('Specify path to the configuration file.')
year = argv[1]&.to_i

begin

if options[:outputfile]
output_dir = File.dirname(options[:outputfile])
output_filename = File.basename(options[:outputfile])
else
output_dir = nil
output_filename = "index_" + File.basename(config_path, ".xml") + ".html"
Copy link
Owner

Choose a reason for hiding this comment

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

With regard to further development it would be better to only make output directory configurable (and build the default one using the config file's name) and always force the same hardcoded file name (index.html). In future there will be output formats producing multiple files per calendar (this is the reason why ordodo produces a directory and not simply a single file).

end

year = options[:year]

begin
config =
File.open(config_path) do |fr|
Config.from_xml(fr) do |config|
config.year = year if year
if output_dir
config.output_dir = output_dir
else
config.output_dir = "ordo_#{config.year}"
end
config.output_filename = output_filename
end
end
Generator.new(config).call
Expand Down
7 changes: 4 additions & 3 deletions lib/ordodo/config.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,10 @@ def initialize
@locale = :en
@temporale_options = {}
@temporale_extensions = []
@calendars = nil
Copy link
Owner

Choose a reason for hiding this comment

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

initialization of @calendars seems to be deleted by accident.

(In Ruby instance variables, if not explicitly assigned, have nil as default value, so this change has no effect on the code's functioning, but we definitely want to initialize all variables, for the sake of readability.)

Copy link
Author

Choose a reason for hiding this comment

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

True.

@output_directory = nil
@year = upcoming_year
@title = 'Liturgical Calendar'
@output_dir = nil
@output_filename = 'ordodo_out'

@loader = CalendariumRomanum::SanctoraleLoader.new

Expand All @@ -25,7 +25,8 @@ def initialize
:temporale_options,
:temporale_extensions,
:calendars,
:output_directory,
:output_dir,
:output_filename,
:title,
:year

Expand Down
3 changes: 2 additions & 1 deletion lib/ordodo/generator.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@ def call
Outputters::HTML.new(
@config,
templates_dir: 'templates/html',
output_dir: "ordo_#{@config.year}",
output_dir: @config.output_dir,
output_filename: @config.output_filename,
globals: globals,
)
]
Expand Down
6 changes: 6 additions & 0 deletions lib/ordodo/outputters/console.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,12 @@ def <<(record)
puts
end
end

def finish

puts "Ordo written to #{@output_dir}/#{@output_filename}"

end

private

Expand Down
3 changes: 2 additions & 1 deletion lib/ordodo/outputters/html.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@ module Outputters
class HTML < Outputter
def prepare
FileUtils.mkdir_p(@output_dir)
@fw = File.open(File.join(@output_dir, 'index.html'), 'w')
# filename = 'index_' + @output_filename + '.html'
@fw = File.open(File.join(@output_dir, @output_filename), 'w')

@fw.puts header.render(self, **@globals)
end
Expand Down
3 changes: 2 additions & 1 deletion lib/ordodo/outputters/outputter.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,10 @@ module Outputters
class Outputter
extend Forwardable

def initialize(config, output_dir: nil, templates_dir: nil, globals: {})
def initialize(config, output_dir: nil, output_filename:nil, templates_dir: nil, globals: {})
@config = config
@output_dir = output_dir
@output_filename = output_filename
@templates_dir = templates_dir
@globals = globals
end
Expand Down