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 providing a list of characters to escape in PR titles #611

Merged
merged 47 commits into from Aug 27, 2020

Conversation

Happypig375
Copy link
Contributor

@Happypig375 Happypig375 commented Aug 1, 2020

Fixes #609

@Happypig375 Happypig375 marked this pull request as draft August 1, 2020 06:47
@Happypig375

This comment has been minimized.

@jetersen
Copy link
Member

jetersen commented Aug 1, 2020

You have not updated the schema. To do so run yarn generate-schema
https://github.com/release-drafter/release-drafter/runs/934759216#step:4:363

README.md Outdated
| Variable | Description |
| --------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| `$NUMBER` | The number of the pull request, e.g. `42`. |
| `$TITLE` | The title of the pull request, e.g. `Add alien technology`. Any characters matching `change-title-escapes` will be prepended with a backslash so that they will appear verbatim instead of being interpreted as markdown format characters. |
Copy link
Member

Choose a reason for hiding this comment

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

What about escaping @
There are two options according to github/markup#1168

One using zero-width space or html comment <!-- -->. I think the html comment is better as it is easier to "recreate" not everybody knows how to type a zero-width space.

@​jetersen
@jetersen

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like the zero-width space better as it doesn't clutter reading as much. We are auto-generating these lines anyways.

Copy link
Member

Choose a reason for hiding this comment

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

What if user accidentally deletes the line and does not know how to type a zero-width space?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jetersen @ jetersen

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops it's a nbsp

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jetersen

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe using comments is the way to go then.

Copy link
Member

Choose a reason for hiding this comment

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

You can also do this:
@&ZeroWidthSpace;jetersen @​jetersen
@&nbsp;jetersen @ jetersen

&nbsp; is not a good option

Copy link
Contributor Author

Choose a reason for hiding this comment

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

<!----> @​jetersen
&#x2060; @⁠jetersen
&#xFEFF; @jetersen
Comments seem to be the most obvious and short option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same trick can be used for #s.

@Happypig375
Copy link
Contributor Author

Happypig375 commented Aug 1, 2020

I have no idea how to create a test with special PR titles...

@jetersen
Copy link
Member

jetersen commented Aug 1, 2020

I think you can simply write tests that test the function directly. Instead of relying on indirect test workflow.

@Happypig375 Happypig375 marked this pull request as ready for review August 1, 2020 08:48
@Happypig375
Copy link
Contributor Author

@jetersen PR is ready

lib/releases.js Outdated Show resolved Hide resolved
Comment on lines 24 to 65
const changelog = generateChangeLog(pullRequests, baseConfig)
expect(changelog).toEqual('* A1 (#1) @ghost\n* B2 (#2) @ghost\n* Adds missing <example> (#3) @jetersen\n* `#code_block` (#4) @jetersen\n* Fixes #4 (#5) @Happypig375\n* 2*2 should equal to 4*1 (#6) @jetersen\n* Rename __confgs\\confg.yml to __configs\\config.yml (#7) @ghost\n* Adds @nullable annotations to the 1*1+2*4 test in `tests.java` (#0) @Happypig375')
})
it('escapes titles with \\s correctly', () => {
const config = {
...baseConfig,
'change-title-escapes': '\\'
}
const changelog = generateChangeLog(pullRequests, config)
expect(changelog).toEqual('* A1 (#1) @ghost\n* B2 (#2) @ghost\n* Adds missing <example> (#3) @jetersen\n* `#code_block` (#4) @jetersen\n* Fixes #4 (#5) @Happypig375\n* 2*2 should equal to 4*1 (#6) @jetersen\n* Rename __confgs\\\\confg.yml to __configs\\\\config.yml (#7) @ghost\n* Adds @nullable annotations to the 1*1+2*4 test in `tests.java` (#0) @Happypig375')
})
it('escapes titles with \\<*_& correctly', () => {
const config = {
...baseConfig,
'change-title-escapes': '\\<*_&'
}
const changelog = generateChangeLog(pullRequests, config)
expect(changelog).toEqual('* A1 (#1) @ghost\n* B2 (#2) @ghost\n* Adds missing \\<example> (#3) @jetersen\n* `#code_block` (#4) @jetersen\n* Fixes #4 (#5) @Happypig375\n* 2\\*2 should equal to 4\\*1 (#6) @jetersen\n* Rename \\_\\_confgs\\\\confg.yml to \\_\\_configs\\\\config.yml (#7) @ghost\n* Adds @nullable annotations to the 1\\*1+2\\*4 test in `tests.java` (#0) @Happypig375')
})
it('escapes titles with @s correctly', () => {
const config = {
...baseConfig,
'change-title-escapes': '@'
}
const changelog = generateChangeLog(pullRequests, config)
expect(changelog).toEqual('* A1 (#1) @ghost\n* B2 (#2) @ghost\n* Adds missing <example> (#3) @jetersen\n* `#code_block` (#4) @jetersen\n* Fixes #4 (#5) @Happypig375\n* 2*2 should equal to 4*1 (#6) @jetersen\n* Rename __confgs\\confg.yml to __configs\\config.yml (#7) @ghost\n* Adds @<!---->nullable annotations to the 1*1+2*4 test in `tests.java` (#0) @Happypig375')
})
it('escapes titles with @s and #s correctly', () => {
const config = {
...baseConfig,
'change-title-escapes': '@#'
}
const changelog = generateChangeLog(pullRequests, config)
expect(changelog).toEqual('* A1 (#1) @ghost\n* B2 (#2) @ghost\n* Adds missing <example> (#3) @jetersen\n* `#code_block` (#4) @jetersen\n* Fixes #<!---->4 (#5) @Happypig375\n* 2*2 should equal to 4*1 (#6) @jetersen\n* Rename __confgs\\confg.yml to __configs\\config.yml (#7) @ghost\n* Adds @<!---->nullable annotations to the 1*1+2*4 test in `tests.java` (#0) @Happypig375')
})
it('escapes titles with \\<@*_&`# correctly', () => {
const config = {
...baseConfig,
'change-title-escapes': '\\<@*_&`#'
}
const changelog = generateChangeLog(pullRequests, config)
expect(changelog).toEqual('* A1 (#1) @ghost\n* B2 (#2) @ghost\n* Adds missing \\<example> (#3) @jetersen\n* \\`#<!---->code\\_block\\` (#4) @jetersen\n* Fixes #<!---->4 (#5) @Happypig375\n* 2\\*2 should equal to 4\\*1 (#6) @jetersen\n* Rename \\_\\_confgs\\\\confg.yml to \\_\\_configs\\\\config.yml (#7) @ghost\n* Adds @<!---->nullable annotations to the 1\\*1+2\\*4 test in \\`tests.java\\` (#0) @Happypig375')
Copy link
Member

Choose a reason for hiding this comment

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

replace toEqual with toMatchInlineSnapshot

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does toMatchInlineSnapshot allow indentation? There is

expect(body).toMatchInlineSnapshot(`
Object {
"body": "# What's Changed
* Add documentation (#5) @TimonVS
* Update dependencies (#4) @TimonVS
* Bug fixes (#3) @TimonVS
* Add big feature (#2) @TimonVS
* 👽 Add alien technology (#1) @TimonVS
",
"draft": true,
"name": "",
"prerelease": false,
"tag_name": "",
}
`)

but this PR's tests seem to fail.

Copy link
Member

@jetersen jetersen Aug 1, 2020

Choose a reason for hiding this comment

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

Because it is inside a object :)

When you use toMatchInlineSnapshot on a string, it cannot tolerate it 😅

Copy link
Member

Choose a reason for hiding this comment

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

You can also use toMatchSnapshot which will create a file where indentation shouldn't be an issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will wrap it in an object

Copy link
Contributor Author

Choose a reason for hiding this comment

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

    - Snapshot  - 13
    + Received  + 10

    -
    -         Object {
    -           "log": "
    -         * A1 (#1) @ghost
    -         * B2 (#2) @ghost
    -         * Adds missing \<example> (#3) @jetersen
    -         * \`\#code\_block\` (#4) @jetersen
    -         * Fixes #<!--->4 (#5) @Happypig375
    -         * 2\*2 should equal to 4\*1 (#6) @jetersen
    -         * Rename \_\_confgs\\confg.yml to \_\_configs\\config.yml (#7) @ghost
    -         * Adds @<!--->nullable annotations to the 1\*1+2\*4 test in `tests.java` (#0) @Happypig375
    -         ",
    -         }
    + Object {
    +   "log": "* A1 (#1) @ghost
    + * B2 (#2) @ghost
    + * Adds missing \\<example> (#3) @jetersen
    + * \\`#<!---->code\\_block\\` (#4) @jetersen
    + * Fixes #<!---->4 (#5) @Happypig375
    + * 2\\*2 should equal to 4\\*1 (#6) @jetersen
    + * Rename \\_\\_confgs\\\\confg.yml to \\_\\_configs\\\\config.yml (#7) @ghost
    + * Adds @<!---->nullable annotations to the 1\\*1+2\\*4 test in \\`tests.java\\` (#0) @Happypig375",
    + }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll throw away indentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

    - Snapshot  - 0
    + Received  + 0

???

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Trying to use toMatchInlineSnapshot is literally a waste of time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jetersen Any suggestions?

@jetersen
Copy link
Member

jetersen commented Aug 4, 2020

I simply did yarn jest --updateSnapshot it's not that hard. Use the lovely tooling people did.

@Happypig375
Copy link
Contributor Author

@jetersen Are there any more issues to resolve with this PR?

@Happypig375
Copy link
Contributor Author

ping

@jetersen jetersen merged commit d472269 into release-drafter:master Aug 27, 2020
@jetersen jetersen added the type: feature New feature or request label Oct 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Escape titles
2 participants