Optparse and Console output #3
Conversation
|
Are there any questions left? |
There was a problem hiding this comment.
Thank you very much, both for the code and for patience.
There are some issues - a few to be fixed, others to be discussed - see below.
Apart of that, I would like to ask for no trailing whitespace and consistent 2-space indentation. I mostly get this done by Emacs autoindentation, other editors have similar features. I'm not opposed to adopting something like rufo or rubocop auto-correct as pre-commit hook, if preferred. I will configure a CI check to enforce sensible coding style.
|
|
||
| options[:year] = nil | ||
| opts.on( '-y', '--year YEAR', 'Specify the YEAR for which the ordo should be produced' ) do|year| | ||
| options[:year] = Integer(year) |
There was a problem hiding this comment.
More idiomatic Ruby way of "type-casting" is by calling the subject's method: year.to_i
| end | ||
|
|
||
| options[:year] = nil | ||
| opts.on( '-y', '--year YEAR', 'Specify the YEAR for which the ordo should be produced' ) do|year| |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
You are right. I not only failed to understand my own code, but also to read my own README 😊
| output_filename = File.basename(options[:outputfile]) | ||
| else | ||
| output_dir = nil | ||
| output_filename = "index_" + File.basename(config_path, ".xml") + ".html" |
There was a problem hiding this comment.
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).
| @@ -4,10 +4,10 @@ def initialize | |||
| @locale = :en | |||
| @temporale_options = {} | |||
| @temporale_extensions = [] | |||
| @calendars = nil | |||
There was a problem hiding this comment.
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.)
|
Ok, I will have a look on your requests and improve the code. I think I will we need some days - I'm quite busy at the moment. |
Hi,
so I finally got into Ruby and your code. Here is my first commit.
Former:
$ordodo example.xml 2050Now:
$ordodo -y 2050 example.xml$ordodo -o path/to/file.html example.xml$ordodo example.xmlwill now result into an outputordo_YEAR/index_example.htmlI tested everything, should be working.
What is missing is changing the README. I wanted to wait for your feedback.
Regards,
martindonat