Add option --configure-sections, --add-sections, --include-merged #587

eputnam · 2017-11-07T05:35:12Z

There's a lot in this PR.

Added a Section class to more easily make the other changes and
hopefully add flexibility for the future
Added an option called configure_sections that allows you create
your own custom sections. It blows away all other sections and uses
only the ones you give it.
Added an option called add_sections that allows you to add_sections
to the default section set
Added an option called include_merged that can be used when
configure_sections is defined. Configure sections blows away any and
all default sections so to get this one back, you have to set this
option.
Added tests for this stuff

@HAIL9000 was a co-author. Because of a little git snafu, I accidentally
squashed all of our work into one so it looks like it was just me.

HAIL9000 · 2017-11-07T07:24:28Z

This looks so good! Thank you for running with it. Should we add our new options to the docs...?

olleolleolle · 2017-11-07T07:58:21Z

This is a big PR. It may take a few turns of code review to get into the codebase, but don't be discouraged by that.

@HAIL9000 is right, there are a few places where all configuration options are to be placed. That is, duplicated in different formats.

olleolleolle · 2017-11-11T15:01:25Z

There are a few style details to polish (later) in this code. RuboCop has its set of complaints.

If we can get more use out of the class Section, I'd be happy. It seems a very good addition to the codebase. Just looking quickly, perhaps a query method Section#merged? would help in some places?

Could the presence of the class Section signal the need for a Release, as well?

eputnam · 2017-11-16T04:02:52Z

@olleolleolle can you elaborate a little on both of those? i'm not sure i see the need for the merged? function or how the signaling for a release would work.

hunner · 2017-12-01T20:27:41Z

I have fixed the rubocop failures and reorganized code relating to generating specific releases and sections of each release out into their own classes outside of the main generation code. The methods otherwise remain mostly the same (a few naming tweaks because "section" and "log" were overloaded terms).

This is still a Work In Progress as I would like to add more tests.

hunner · 2017-12-01T23:54:24Z

lib/github_changelog_generator/generator/entry.rb

+    # @param [Hash, nil] older_tag Older tag, used for the links. Could be nil for last tag.
+    # @return [String] Ready and parsed section
+    def create_entry_for_tag(pull_requests, issues, newer_tag, older_tag = nil)
+      newer_tag_link, newer_tag_name, newer_tag_time = detect_link_tag_time(newer_tag)


Should an entry be told what the tags link/name/time is, or should it be able to figure it out just given a tag?

If the former, this logic moves up to the generator. If the latter, the methods for tag munging need to move into the entry class.

The latter would be the better design decision, but get_time_of_tag is called by detect_link_tag_time and used in multiple other places, so moving detect_link_tag_time to the Entry class would cause issues around that.

Separating the tag utility methods out into their own GeneratorTags module instead of being part of either the Generator or Entry classes would be preferable, but is beyond the scope of this PR.

The newer_tag values of name/link/time should be consistently handled in that order inside the Entry class. I have done some small refactoring to make this so.

hunner · 2017-12-01T23:55:44Z

lib/github_changelog_generator/generator/entry.rb

+      # the SHA for the first commit.
+      older_tag_name =
+        if older_tag.nil?
+          @fetcher.commits_before(newer_tag_time).last["sha"]


Should an entry fetch it's preceding commits, or is this logic part of the larger changelog generation?

If the former, then the entry class needs access to the fetcher (pass through options?). If the latter, then this needs to move out of the entry class back to the generator class.

The create_entry_for_tag method should not make calls out to github, so instead of having the fetcher passed through in the case of a nil older tag, the older tag name should be a required parameter and the generator class should either pass the older tag name or the first commit sha in the absence of an older tag.

The older_tag_link variable used in generate_header is misnamed since it is actually just a tag link, not a name. It is also possibly a sha and not a tag, but I'll ignore that for now.

hunner · 2017-12-01T23:56:19Z

lib/github_changelog_generator/generator/entry.rb

+
+      @content
+    end
+


All following methods should be private, and create_entry_for_tag should be tested.

hunner · 2017-12-04T18:25:47Z

lib/github_changelog_generator/generator/section.rb

+      content = ""
+
+      if issues.any?
+        content += "#{prefix}\n\n" unless @options[:simple_list]


The @options hash is scattered around the various classes and passed around as needed but never modified once parsed. It would be nice if the options were centrally accessible instead of passing them around, but that is out of scope for this PR.

Instead, I'll just make simple_list an invocation option to be removed later.

Eh, there are several more options needed by the Section class. I guess I'll pass @options through and it can be refactored in another PR.

hunner · 2017-12-04T22:44:45Z

@olleolleolle Green and ready for your review again \o/.

eputnam · 2017-12-05T18:55:27Z

tagging @kvolkovich-sc since you seem to have some interest in the feature. feel free to have a look at what we've put together!

skywinder · 2017-12-06T13:00:13Z

Very powerful PR. Thank you all for your efforts, @eputnam @hunner 🎉

olleolleolle

I was looking at this the other morning, from the top of the changes, making small notes as I went, if you find the time, perhaps you can address these. If not, that's fine, too.

olleolleolle · 2017-12-05T05:30:56Z

lib/github_changelog_generator/generator/entry.rb

+        map[section.name] = section
+      end
+
+      map


each_with_object opportunity here.

Good catch!

olleolleolle · 2017-12-05T05:32:11Z

lib/github_changelog_generator/generator/entry.rb

+    # @param [Time] newer_tag_time - time, when newer tag created
+    # @param [String] older_tag_name - tag name, used for links.
+    # @param [String] project_url - url for current project.
+    # @return [String] - Generate one ready-to-add section.


Totally tiny thing: the return value should be described as a noun not a verb.

olleolleolle · 2017-12-05T05:33:00Z

lib/github_changelog_generator/generator/entry.rb

+    # Generates complete body text for a tag (without a header)
+    #
+    # @param [Array] issues
+    # @param [Array] pull_requests


These two, they could be in the order the positional parameters are.

Okay. I updated the methods to take the order pull_requests, issues as well for consistency.

olleolleolle · 2017-12-05T05:33:43Z

lib/github_changelog_generator/generator/entry.rb

+          content += section.generate_content
+        end
+
+        content


Another each_with_object opportunity

olleolleolle · 2017-12-05T05:35:01Z

lib/github_changelog_generator/generator/entry.rb

+    # Boolean method for whether the user is using configure_sections
+    def configure_sections?
+      !@options[:configure_sections].nil? && !@options[:configure_sections].empty?
+    end


If you want you can move these methods to the Options class. It looks like a Hash but it is a class.

olleolleolle · 2017-12-05T05:35:32Z

lib/github_changelog_generator/generator/entry.rb

+      ]
+    end
+
+    # This method sort issues by types


Typo sort -> sorts

olleolleolle · 2017-12-05T06:10:29Z

lib/github_changelog_generator/generator/entry.rb

+        end
+      end
+      added_pull_requests.each { |p| pull_requests.delete(p) }
+      @sections


The local variable name p, could you make it something else?

https://ruby-doc.org/core-2.4.2/Kernel.html#method-i-p is a method, and some folks' eyes read that p as a method rather than as a local variable.

hunner · 2017-12-07T00:18:33Z

lib/github_changelog_generator/generator/entry.rb

+    def generate_body(pull_requests, issues)
+      body = ""
+      body += main_sections_to_log(pull_requests, issues)
+      body += merged_section_to_log(pull_requests) if (@options[:pulls] && @options[:add_pr_wo_labels]) || (@options.configure_sections? && @options[:include_merged])


It looks like the logic here is not correct. :include_merged defaults to true, but seems like it should default to false. Additionally, the :configure_section option does not override the :add_pr_wo_labels option as it appears to be meant to do.

The UX here needs a little thinking through.

hunner · 2017-12-08T16:58:11Z

Okay, it looks like the add_pr_wo_labels option applies to normal generation and custom section generation now. No include_merged option needed.

eputnam · 2017-12-08T22:02:02Z

ready for you again, @olleolleolle @skywinder

olleolleolle

I read this, and made more notes.

Thanks for keeping up with all this.

olleolleolle · 2017-12-08T22:22:23Z

lib/github_changelog_generator/parser_file.rb

-      github_api: :github_endpoint
+      github_api: :github_endpoint,
+      configure_sections: :configure_sections,
+      add_sections: :add_sections


These are only needed here if they aren't the same - as you can see, all the other pairs are about irregular names.

So, do not add the new option names here.

olleolleolle · 2017-12-08T22:23:40Z

spec/unit/generator/entry_spec.rb

@@ -0,0 +1,351 @@
+# frozen_string_literal: true
+
+describe GitHubChangelogGenerator::Entry do


Please use the form RSpec.describe (not describe) for the outermost describe.

You can structure tests inside a module, like this:

module GitHubChangelogGenerator RSpec.describe Entry do # ...in here, names will be in the namespace of the module. # so that you can use shorter names. end end

olleolleolle · 2017-12-08T22:25:10Z

spec/unit/generator/entry_spec.rb

+    end
+  end
+  describe "#parse_sections" do
+    before :each do


:each is deprecated RSpec syntax.

That setting is today called :example, but it is also the default.

So you can say before do ... end instead.

olleolleolle · 2017-12-08T22:29:52Z

spec/unit/generator/entry_spec.rb

+          expect(sections_array[i].labels).to eq sections_json.first[1]["labels"]
+          expect(sections_array[i].issues).to eq []
+          sections_json.shift
+        end


This place is a candidate for using aggregate_failures, the RSpec feature.

There are many expects in the same it here, and if there's a failure, things stop executing.

Look that feature up, it's quite cool.

olleolleolle · 2017-12-08T22:30:22Z

spec/unit/generator/entry_spec.rb

+          expect(sections_array[i].labels).to eq sections_hash.first[1][:labels]
+          expect(sections_array[i].issues).to eq []
+          sections_hash.shift
+        end


Right, here's another it with many expects.

olleolleolle · 2017-12-08T22:32:33Z

spec/unit/generator/entry_spec.rb

+    }
+  end
+
+  def get_titles(issues)


Could we change this name to be more about the result of the call?

Example method name:

titles_for(issues)

olleolleolle · 2017-12-08T22:33:13Z

spec/unit/generator/entry_spec.rb

+        expect(get_titles(enhancement_section.issues)).to eq(["pr enhance"])
+        expect(get_titles(bug_section.issues)).to eq(["issue bugaboo"])
+        expect(get_titles(pull_requests)).to eq(["pr no labels"])
+      end


Many expects here, too.

olleolleolle · 2017-12-08T23:51:14Z

spec/unit/generator/entry_spec.rb

+        let(:sections_array) do
+          [
+            GitHubChangelogGenerator::Section.new(name: "foo", prefix: "foofix", labels: %w[test1 test2]),
+            GitHubChangelogGenerator::Section.new(name: "bar", prefix: "barfix", labels: %w[test3 test4])


Now, it ought to be possible to just say "Section.new(...)" here.

olleolleolle · 2017-12-09T00:11:08Z

lib/github_changelog_generator/generator/entry.rb

+
+    def initialize(options = {})
+      @content = ""
+      @options = Options.new(options)


Could we decide that it must be an Options that is passed in here?

hunner · 2017-12-14T18:22:08Z

Rebased to resolve conflicts. Anything else you'd like to see?

olleolleolle · 2017-12-14T18:50:21Z

@hunner One small thing!

We really could have some doc blocks on these new classes, with more-descriptive words, such as this great section:

I reasoned that a changelog is a series of "entries" of all tagged
releases plus an extra entry for the unreleased entry. Each entry is
comprised of a header and a series of "sections" for that entry. Each
section is comprized of a list of issues and/or pull requests for that
entry. So the log contains entries, entries contain sections, and
sections contain issues & prs. I have structured the classes around this idea.

olleolleolle · 2017-12-14T19:15:37Z

lib/github_changelog_generator/generator/entry.rb

+      begin
+        sections_json = JSON.parse(sections_desc)
+      rescue JSON::ParserError => e
+        raise "There was a problem parsing your JSON string for secions: #{e}"


Typo: secions => sections

olleolleolle · 2017-12-14T19:27:23Z

lib/github_changelog_generator/generator/entry.rb

+        sections_arr << Section.new(name: name.to_s, prefix: v["prefix"], labels: v["labels"], options: @options)
+      end
+
+      sections_arr


We can leave off naming a local variable here, and only return the result of #map

sections_json.map do |name, v| Section.new(name: name.to_s, prefix: v["prefix"], labels: v["labels"], options: @options) end

@HAIL9000

There's a lot in this PR. - Added a Section class to more easily make the other changes and hopefully add flexibility for the future - Added an option called `configure_sections` that allows you create your own custom sections. It blows away all other sections and uses only the ones you give it. - Added an option called `add_sections` that allows you to add_sections to the default section set - Added an option called `include_merged` that can be used when configure_sections is defined. Configure sections blows away any and all default sections so to get this one back, you have to set this option. - Added tests for this stuff @HAIL9000 was a co-author. Because of a little git snafu, I accidentally squashed all of our work into one so it looks like it was just me. --- Refactor details: Before this change, the code in generator.rb and generator_generation.rb was conflated and method call flow went back and forth between the two files seemingly randomly. They also both defined the exact same class, which is un-ruby-ish. I tried to separate methods used for the whole changelog generation from methods used for specific parts of the changelog and move them into specific classes. I reasoned that a changelog is a series of "entries" of all tagged releases plus an extra entry for the unreleased entry. Each entry is comprised of a header and a series of "sections" for that entry. Each section is comprized of a list of issues and/or pull requests for that entry. So the log contains entries, entries contain sections, and sections contain issues & prs. I have structured the classes around this idea. - lib/github_changelog_generator/generator/generator.rb is for code related to generating the entire changelog. - lib/github_changelog_generator/generator/entry.rb is for code related to generating entries. - lib/github_changelog_generator/generator/section.rb is for code relating to geneating entry sections. Issues and PRs are already special objects, so it doesn't make sense to break those out into their own class.

olleolleolle · 2017-12-15T15:23:36Z

It's been a marathon, all!

👍

hunner · 2017-12-22T03:56:15Z

Thank you for working with us to get this merged too; and thank you for such commitment to maintaining this useful tool!

eputnam force-pushed the config_sections branch 3 times, most recently from 04e7624 to 675b032 Compare November 7, 2017 06:12

kvolkovich-sc mentioned this pull request Dec 1, 2017

Add CHANGELOG.md generation/update during release OpusCapita/filemanager#111

Closed

hunner force-pushed the config_sections branch 3 times, most recently from c682755 to 265b089 Compare December 1, 2017 20:16

hunner suggested changes Dec 1, 2017

View reviewed changes

hunner reviewed Dec 4, 2017

View reviewed changes

hunner force-pushed the config_sections branch 2 times, most recently from a324d63 to e812a57 Compare December 4, 2017 22:31

hunner force-pushed the config_sections branch 2 times, most recently from 0dbdaa7 to 3e490fe Compare December 4, 2017 22:50

olleolleolle reviewed Dec 6, 2017

View reviewed changes

hunner suggested changes Dec 7, 2017

View reviewed changes

eputnam force-pushed the config_sections branch 2 times, most recently from 83bd3c7 to 04420ea Compare December 7, 2017 22:50

olleolleolle reviewed Dec 8, 2017

View reviewed changes

olleolleolle reviewed Dec 9, 2017

View reviewed changes

hunner force-pushed the config_sections branch 2 times, most recently from 23e8f34 to 5fefc77 Compare December 14, 2017 18:04

olleolleolle reviewed Dec 14, 2017

View reviewed changes

hunner force-pushed the config_sections branch from 5fefc77 to 587557e Compare December 15, 2017 00:13

olleolleolle changed the title ~~configure custom sections~~ Add option --configure_sections Dec 15, 2017

olleolleolle changed the title ~~Add option --configure_sections~~ Add option --configure_sections, --add_sections, --include_merged Dec 15, 2017

olleolleolle changed the title ~~Add option --configure_sections, --add_sections, --include_merged~~ Add option --configure-sections, --add-sections, --include-merged Dec 15, 2017

olleolleolle merged commit e2ddb73 into github-changelog-generator:master Dec 15, 2017

hunner deleted the config_sections branch December 22, 2017 03:56

hunner mentioned this pull request Feb 20, 2018

Fix unlabeled, mixed labels, and unmapped labels handling #618

Merged

hunner mentioned this pull request Apr 12, 2018

Release prep 2.0.0 #641

Closed

skywinder added this to the 2.0.0 milestone Apr 15, 2018

		@@ -0,0 +1,351 @@
		# frozen_string_literal: true

		describe GitHubChangelogGenerator::Entry do

Add option --configure-sections, --add-sections, --include-merged #587

Add option --configure-sections, --add-sections, --include-merged #587

Conversation

eputnam commented Nov 7, 2017 • edited Loading

HAIL9000 commented Nov 7, 2017

olleolleolle commented Nov 7, 2017

olleolleolle commented Nov 11, 2017

eputnam commented Nov 16, 2017

hunner commented Dec 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hunner commented Dec 4, 2017

eputnam commented Dec 5, 2017

skywinder commented Dec 6, 2017

olleolleolle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olleolleolle Dec 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olleolleolle Dec 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hunner commented Dec 8, 2017

eputnam commented Dec 8, 2017

olleolleolle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hunner commented Dec 14, 2017

olleolleolle commented Dec 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olleolleolle commented Dec 15, 2017

hunner commented Dec 22, 2017

eputnam commented Nov 7, 2017 •

edited

Loading

olleolleolle Dec 5, 2017 •

edited

Loading

olleolleolle Dec 5, 2017 •

edited

Loading