Skip to content

Conversation

@harshil21
Copy link
Member

Checklist for PRs

  • Added .. versionadded:: version, .. versionchanged:: version or .. deprecated:: version to the docstrings for user facing changes (for methods/class descriptions, arguments and attributes)
  • Created new or adapted existing unit tests
  • Added myself alphabetically to AUTHORS.rst (optional)
  • Added new classes & modules to the docs

Any more changes to be done?

Closes #2839

@harshil21 harshil21 added enhancement 📋 pending-review work status: pending-review labels Jan 3, 2022
@harshil21 harshil21 added this to the v14 milestone Jan 3, 2022
Copy link
Member

@Bibo-Joshi Bibo-Joshi left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! left some comments. regarding api_defaults:

It's public, because we access it outside of Defaults in Bot. And it doesn't provide any info that's not available otherwise - you can just check the properties of Defaults. In fact, even the properties are of no actual value for a user …
IMO it's not necessary to make it private and then having to shut up pylint about access to private attributes.
If you prefer, we can even just give it a docstring.

@harshil21 harshil21 removed the 📋 pending-review work status: pending-review label Jan 4, 2022
Copy link
Member

@Bibo-Joshi Bibo-Joshi left a comment

Choose a reason for hiding this comment

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

thanks for the updates!

@harshil21 harshil21 force-pushed the protect-content-defaults branch from 23f75f0 to f81ea04 Compare January 4, 2022 14:19
Copy link
Member

@Bibo-Joshi Bibo-Joshi left a comment

Choose a reason for hiding this comment

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

LGTM! @Poolitzer are you interested in reviewing?

the deepsource failures are unrelated and almost all false positives - let's ignore them on this PR

Copy link
Member

@Poolitzer Poolitzer left a comment

Choose a reason for hiding this comment

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

LGTM!

@harshil21 harshil21 added the 📋 pending-merge work status: pending-merge label Jan 6, 2022
@Bibo-Joshi Bibo-Joshi merged commit d84ad54 into v14 Jan 7, 2022
@Bibo-Joshi Bibo-Joshi deleted the protect-content-defaults branch January 7, 2022 16:02
@harshil21 harshil21 removed the 📋 pending-merge work status: pending-merge label Jan 7, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Jan 14, 2022
@Bibo-Joshi Bibo-Joshi added 🔌 enhancement pr description: enhancement and removed enhancement labels Nov 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

🔌 enhancement pr description: enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants