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.

Sign up for GitHub

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

Jump to bottom

refactor(migrations): validator #13073

Merged
merged 20 commits into from
Jan 10, 2022
Merged

refactor(migrations): validator #13073

merged 20 commits into from
Jan 10, 2022

Conversation

pret-a-porter
Copy link
Contributor

@pret-a-porter pret-a-porter commented Dec 12, 2021 •
edited

Changes:

  1. Introduce custom migration validator to test only one specific migration. It increases unit test speed and reduces code duplication
  2. Add few more migrations from previously huge PR Refactor/migrations #12957
  3. Replace delete method by deprecated field for more declarative way
  4. Add debug task "Jest Current Folder" for vscode, which runs all tests inside selected directory

Context:

  1. Migrate inline with same sort  #11459
  2. Part of Refactor/migrations #12957

Documentation

  • I have updated the documentation, or
  • No documentation update is required

How I've tested my work

I have verified these changes via:

  • Code inspection only, or
  • Newly added/modified unit tests, or
  • No unit tests but ran on a real repository, or
  • Both unit tests + ran on a real repository

@pret-a-porter pret-a-porter changed the title Refactor/migration validator refactor(migrations): validator Dec 12, 2021
viceice
viceice reviewed Dec 13, 2021
View reviewed changes
Copy link
Member

@viceice viceice left a comment

Choose a reason for hiding this comment

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

Please split refactoring from new migration to separate PR.

.eslintrc.js Outdated Show resolved Hide resolved
.vscode/launch.json Outdated Show resolved Hide resolved
@pret-a-porter
Copy link
Contributor Author

Please split refactoring from new migration to separate PR.

I have removed commits with migrations, but unfortunately by force push

viceice
viceice reviewed Jan 3, 2022
View reviewed changes
lib/config/migrations/validator.ts Outdated Show resolved Hide resolved
.eslintrc.js Outdated Show resolved Hide resolved
viceice
viceice reviewed Jan 10, 2022
View reviewed changes
lib/config/migrations/base/remove-property-migration.ts Outdated
@@ -13,7 +13,7 @@ export class RemovePropertyMigration extends AbstractMigration {
this.propertyName = propertyName;
}

override run(): void {
run(): void {
Copy link
Member

Choose a reason for hiding this comment

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

Why override is no longer required?

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 decided that is not correct to use override keyword for methods, which is marked as abstract in parent class

Copy link
Member

Choose a reason for hiding this comment

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

Can you check if we set the noImplicitOverride typescript option? It should be enabled and force the override usage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it is already enabled 5 month ago

Copy link
Member

Choose a reason for hiding this comment

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

I think we should leave the override, so we can't get into trouble in future

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Returned

lib/config/migrations/custom/binary-source-migration.spec.ts Show resolved Hide resolved
viceice
viceice reviewed Jan 10, 2022
View reviewed changes
Copy link
Member

@viceice viceice left a comment

Choose a reason for hiding this comment

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

See comments

viceice
viceice approved these changes Jan 10, 2022
View reviewed changes
Copy link
Member

@viceice viceice left a comment

Choose a reason for hiding this comment

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

🥳

@viceice viceice enabled auto-merge (squash) January 10, 2022 21:01
@viceice viceice merged commit 6e94385 into renovatebot:main Jan 10, 2022
@renovate-release
Copy link
Collaborator

🎉 This PR is included in version 31.23.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@pret-a-porter pret-a-porter deleted the refactor/migration_validator branch January 11, 2022 20:27
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@viceice viceice viceice approved these changes

@rarkins rarkins Awaiting requested review from rarkins

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@pret-a-porter @renovate-release @viceice @rarkins