search

igneus / calendarium-romanum

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

Improve `calendariumrom query` #7

Closed igneus closed 4 years ago

igneus commented 6 years ago

EDIT October 2019: no more significant features are planned for the calendariumrom query subcommand - bug fixes are still most welcome, but a new, feature-rich CLI for calendarium-romanum is being developed as a separate gem https://github.com/calendarium-romanum/calrom. Feature requests and/or contributions should be directed there.

The gem ships an executable calendariumrom, whose functionality is defined in class CalendariumRomanum::CLI.

Subcommand query enables the user to query liturgical calendar from the commandline. This subcommand could be extended and improved:

Separate PRs are preferred over a huge one implementing all or several of the suggested features. Proposals of other features are most welcome.

simonszu commented 6 years ago

Just a small proposal, maybe not even a feature but rather a bugfix: calendariumrom query outputs the season as a Ruby object, which is not very human readable:

$ calendariumrom query                                                                    [16:17:32]
2017-10-03
season: #<CalendariumRomanum::Season:0x000055e8fd0d3d60>

ferial : Tuesday, 26th week in Ordinary Time

Is this the desired behaviour, or should this be fixed?

If desired, i think i can fix this. However, since it's Hacktoberfest and i am new to all the collaborative features in git and github (just used it for myself until now), i'd like to ask: What is the usual workflow from now on? I have branched my local copy, pushed the changes in the branch to my fork and created a pull request. Until you haven't merge this pull request, i would now:

  1. Switch back to the master branch
  2. Checkout a second branch
  3. Commit and push the second branch to my forked repo and create a pull request for the second branch
  4. Merge the first branch in my fork when you merge it in upstream, and be happy.

Is this the correct workflow? But what, if i, for example, need a work from a previously committed and pull-requested, but not yet upstream merged branch for a new-feature branch? Should i just branch from the first branch instead of the master? Will git correctly merge it, if i have several dependent branches, and, e.g. branch A is branched from master, and branch B is branched from A, and i merge B into master, but not A?

igneus commented 6 years ago

Hi @simonszu , the output is not intended. It's a bug introduced as a side effect of https://github.com/igneus/calendarium-romanum/commit/9e33ca478416ec287f7d402151e1bddb44f22076 Fix will be most welcome.

The workflow you propose is almost correct. Just instead of 4 you should pull from the upstream repository (add it as a new remote repo to your local clone).

But what, if i, for example, need a work from a previously committed and pull-requested, but not yet upstream merged branch for a new-feature branch? Should i just branch from the first branch instead of the master?

Then you could base a new branch on the previously "pull-requested" one and make a notice in the subsequent PR's description "based on previous pull-request #N". There's an inherent danger (danger of your work going in vain) in this scenario: if the first PR doesn't get merged for whatever reason, the subsequent one won't be merged either, or I will ask you to perform some rebasing or cherry-picking gymnastics in order to submit only a subset of the changes, without the unwanted ones.

simonszu commented 6 years ago

Hey, @igneus I have worked out an ability to specify a custom calendar file to load from file system. I need your suggestions for writing tests for this.

I'd like to add a file with an invalid calendar format for the test. The test should test the cli command to fail when loading this invalid data file. But where should i save this file? Is it okay to create a directory spec/data and store this file therein?

For testing the loading of a valid file from the file system i can use the filesystem path to a builtin calendar, i think.

igneus commented 6 years ago

@simonszu

Is it okay to create a directory spec/data and store this file therein?

As the example file will be tiny, it's preferable to use aruba's write_file helper and have complete contents of the file directly in cli_spec.rb. I have used this approach when writing specs for the errors and cmp subcommands.

For testing the loading of a valid file from the file system i can use the filesystem path to a builtin calendar, i think.

Sure.

igneus commented 4 years ago

Closing. It is not planned to add any substantial new features to calendariumrom query.