Skip to content

feanil/cleanup unused code - #206

Merged
feanil merged 3 commits into
masterfrom
feanil/cleanup_unused_code
Oct 11, 2022
Merged

feanil/cleanup unused code#206
feanil merged 3 commits into
masterfrom
feanil/cleanup_unused_code

Conversation

@feanil

@feanil feanil commented Oct 7, 2022

Copy link
Copy Markdown
Contributor
  • docs: Fix the spacing.
  • feat!: Remove the other_emails field.
  • feat!: Remove the jira field.
  • feat!: Remove core committer champion pings on PRs.

Feanil Patel added 3 commits October 6, 2022 15:35
Feanil Patel
feat!: Remove the other_emails field.
c711364 
We don't seem to be using this field for anything so remove it from the
tests and comments.

BREAKING CHANGE: If you have a `other_emails` list in people.yaml it
will be ignored.
Feanil Patel
feat!: Remove the jira field.
ddb3dd2 
We don't seem to be using this field for anything so remove it from the
test and comments.

BREAKINg CHANGE: If you have a `jira` text field in people.yaml it will
be ignored.
Feanil Patel
feat!: Remove core committer champion pings on PRs.
1f4d70c 
We no longer have champions for core committers. So remove code related
to champions.

See https://discuss.openedx.org/t/changes-to-committer-champion-program/6279
for more context.

BREAKING CHANGE: The `champions` key in the `committer` dict of a person
in people.yaml is now ignored.
@feanil
feanil requested a review from nedbat October 7, 2022 18:59
@codecov

codecov Bot commented Oct 7, 2022

Copy link
Copy Markdown

Codecov Report

Base: 90.40% // Head: 90.35% // Decreases project coverage by -0.04% ⚠️

Coverage data is based on head (1f4d70c) compared to base (4173d42).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master     #206      +/-   ##
==========================================
- Coverage   90.40%   90.35%   -0.05%     
==========================================
  Files          72       72              
  Lines        4303     4281      -22     
  Branches      472      468       -4     
==========================================
- Hits         3890     3868      -22     
  Misses        388      388              
  Partials       25       25
Impacted Files Coverage Δ
openedx_webhooks/bot_comments.py 100.00% <ø> (ø)
openedx_webhooks/info.py 97.54% <ø> (ø)
openedx_webhooks/tasks/pr_tracking.py 97.13% <ø> (-0.09%) ⬇️
tests/test_info.py 100.00% <ø> (ø)
tests/test_pull_request_closed.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@nedbat

nedbat commented Oct 7, 2022

Copy link
Copy Markdown
Contributor

I don't see docs: fix the spacing in the changes.

@nedbat

nedbat commented Oct 7, 2022

Copy link
Copy Markdown
Contributor

Should we update repo-tools-data-schema to get rid of fields we aren't using anymore?

@nedbat

nedbat commented Oct 7, 2022

Copy link
Copy Markdown
Contributor

I wonder about other_emails: that was because we'd know their non-work email first, and then get edx.org emails, and we didn't want to lose information. Also, it helped figure people out if they were committing from different GitHub accounts.

But we aren't maintaining it anymore, so I guess it should go.

nedbat
nedbat approved these changes Oct 7, 2022
View reviewed changes

@nedbat nedbat left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The changes seem good as far as they go, but what's the plan for repo-tools-data-schema?

@feanil feanil mentioned this pull request Oct 11, 2022
Open
@feanil

feanil commented Oct 11, 2022

Copy link
Copy Markdown
Contributor Author

We'll have to update repo-tools-data-schema in the same way I guess. I added openedx/openedx-webhooks-data-schema#18 to follow up in that repo.

@feanil
feanil merged commit c37ba1d into master Oct 11, 2022
@feanil
feanil deleted the feanil/cleanup_unused_code branch October 11, 2022 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@nedbat nedbat nedbat approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@feanil @nedbat