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

feat: Validate PR description Changelogs on Eigen #183

Closed
wants to merge 8 commits into from

Conversation

MounirDhahri
Copy link
Member

This PR resolves the following RFC: Validate PR description Changelogs on Eigen

In order to not break our current workflows in Eigen, this rule will be temporarily running only on Eigen PRs containing #run_new_changelog_check in the PR body. Once we finish the remaining work on Eigen, we can safely enable it again for all PRs in Eigen.

@MounirDhahri MounirDhahri changed the title Validate PR description Changelogs on Eigen feat: Validate PR description Changelogs on Eigen May 19, 2021
Comment on lines +3 to +7
import { ParseResult } from "./helpers/changelog/changelog-types"
// @ts-ignore
import { parsePRDescription } from "./helpers/changelog/parsePRDescription.js"
// @ts-ignore
import { changelogTemplateSections } from "./helpers/changelog/generateChangelogSectionTemplate.js"
Copy link
Member Author

Choose a reason for hiding this comment

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

I think it might be better if I just create a new repo with these files to consume on both Eigen and here

import yarn from "danger-plugin-yarn"
import { ParseResult } from "./helpers/changelog/changelog-types"
// @ts-ignore
import { parsePRDescription } from "./helpers/changelog/parsePRDescription.js"
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 ts-ignore here?

Copy link
Member Author

Choose a reason for hiding this comment

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

typescript had issues importing javascript files. Maybe I can try to update the typescript config file to fix that as well

}
`)
})
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Great tests 🙌

Copy link
Contributor

@brainbicycle brainbicycle left a comment

Choose a reason for hiding this comment

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

This looks great to me 🤩 Going to assign @dblandin since he is more familiar with the repo and conventions here but nice work!!!

@MounirDhahri
Copy link
Member Author

closing this since we can achieve the same behaviour on Eigen. This is the link for the PR artsy/eigen#4815

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.

None yet

3 participants