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.

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

Allow format issue output text within individual sections and in general #103

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

Dinozavvvr
Copy link

Hi everyone! Here is a simple solution for issue #47

  • Allow format issue output text within individual sections and in general
  • Removed generateLinks flag

@wilkinsona
Copy link
Contributor

Removed generateLinks flag

Even if a custom format allows the links to be omitted, having a flag that controls the behavior when using the default format is a useful convenience that I think should be kept.

@Dinozavvvr
Copy link
Author

@wilkinsona Could you describe the scenario of how format should work in the case where it has ${number} but the generateLinks flag is set to false. I suggest another way to eliminate these conflicts by setting the default-format parameter to ${title}, which is equivalent to generateLinks=true.

Another option would be to use generateLinks=true to internally set the default format to ${title}, but not allow generateLinks and default-format to be set at the same time.

Thank you for your feedback, please say me if you have any other ideas

@wilkinsona
Copy link
Contributor

I would only apply generateLinks to the default format and ignore it when a custom format has been supplied.

When using a custom format, I don't think ${number} should generate a link. It should only output the issue's number. If you want a link, I would expect the format to be something like ${title} [${number}](${url}). This should provide complete control over the link and its text. You could then also do something like [${title} ${number}](${url}) if you want the issue's title to be part of the link text.

@Dinozavvvr
Copy link
Author

Hi again @wilkinsona. I've put generate-links back the way it worked before. Now, if default-format is explicitly specified, the generate-links flag is ignored. If default-format is not specified, generate-links then "${title}" will be used as the default format, as it worked before. Is it what you wanted?

@Dinozavvvr

This comment was marked as resolved.

@snicoll

This comment was marked as resolved.

Comment on lines +19 to +22
import static io.spring.githubchangeloggenerator.ApplicationProperties.FormatPlaceholder.*;
import static io.spring.githubchangeloggenerator.ApplicationProperties.FormatPlaceholder.TITLE;

import io.spring.githubchangeloggenerator.github.service.Repository;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't re-order the imports. It adds noise to the diff, making the changes harder to review. If you run ./gradlew build, you will see failures from both Checkstyle and Spring Java Format that should be corrected please.

Comment on lines +80 to +81
@DefaultValue("false") boolean addSections) {

Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like a whitespace-only change. Please revert.

Comment on lines +147 to +150
/**
* Whether to generate a link to each issue in the changelog.
*/
private final boolean generateLinks;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need a second property for this? The Issues inner-class already has one.

public Issues(IssueSort sort, IssuesExclude exclude, Set<PortedIssue> ports,
@DefaultValue("true") boolean generateLinks) {
@DefaultValue("true") Boolean generateLinks, String defaultFormat) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why has generatedLinks been changed from boolean to Boolean?

Comment on lines +19 to +30
import static io.spring.githubchangeloggenerator.ApplicationProperties.FormatPlaceholder.NUMBER;
import static io.spring.githubchangeloggenerator.ApplicationProperties.FormatPlaceholder.TITLE;
import static io.spring.githubchangeloggenerator.ApplicationProperties.FormatPlaceholder.URL;

import io.spring.githubchangeloggenerator.ApplicationProperties.ExternalLink;
import io.spring.githubchangeloggenerator.ApplicationProperties.IssueSort;
import io.spring.githubchangeloggenerator.ApplicationProperties.PortedIssue;
import io.spring.githubchangeloggenerator.github.payload.Issue;
import io.spring.githubchangeloggenerator.github.payload.Label;
import io.spring.githubchangeloggenerator.github.payload.User;
import io.spring.githubchangeloggenerator.github.service.GitHubService;
import io.spring.githubchangeloggenerator.github.service.Repository;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please correct the imports as per the problems reported by Checkstyle when you run ./gradlew build.

Map<ChangelogSection, List<Issue>> sectionIssues) {
sectionIssues.forEach((section, issues) -> {
sort(section.getSort(), issues);
content.append((!content.isEmpty()) ? String.format("%n") : "");
Copy link
Contributor

Choose a reason for hiding this comment

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

Switching to isEmpty() is unrelated to allowing more control over the formatting. Please revert.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants