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 clean_content #2850

Merged
merged 1 commit into from
Jul 10, 2024
Merged

Refactor clean_content #2850

merged 1 commit into from
Jul 10, 2024

Conversation

JoeyStk
Copy link
Contributor

@JoeyStk JoeyStk commented Jun 16, 2024 •
edited
Loading

Short description

This PR attempts to clean up the code of clean_content a little bit. As this is my very first attempt at refactoring empathic and honest feedback is much appreciated. In general I tried to follow instructions and ideas from "Clean Code" by Robert C. Martin, but I'm not 100% sure if I was successful.

Proposed changes

  • Split up clean_content into multiple sub-methods
  • Remove pylint ignores
  • Write a test for the method and it's sub-methods

Side effects

  • None that I'm aware of. Thorough double-checking is much appreciated.

Resolved issues

Fixes: #2849

Pull Request Review Guidelines

@codeclimate CodeClimate
Copy link

codeclimate bot commented Jun 16, 2024 •
edited
Loading

Code Climate has analyzed commit 281040e and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 78.2% (50% is the threshold).

This pull request will bring the total coverage in the repository to 83.1% (0.1% change).

View more on Code Climate.

david-venhoff
david-venhoff reviewed Jun 17, 2024
View reviewed changes
Copy link
Member

@david-venhoff david-venhoff left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Could you extract this functionality into its own file? Right now it is required to construct the form to clean content strings, but that is not really necessary.
Decoupled content cleaning is for example required for #2530 (here) and the feature/add-ical-subscription branch

@JoeyStk
Copy link
Contributor Author

JoeyStk commented Jun 17, 2024

Could you extract this functionality into its own file?

Do you mean all of the methods or just one in particular? 🤔

@david-venhoff
Copy link
Member

only the functionality to clean the content field :)

@JoeyStk JoeyStk force-pushed the enhancement/refactor-custom-content-model-form branch from 0596357 to 8f406e0 Compare June 24, 2024 14:12
david-venhoff
david-venhoff reviewed Jun 24, 2024
View reviewed changes
Copy link
Member

@david-venhoff david-venhoff left a comment

Choose a reason for hiding this comment

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

Thanks, this looks mostly good. I have some ideas for improvement

integreat_cms/cms/forms/custom_content_model_form.py Outdated Show resolved Hide resolved
integreat_cms/cms/utils/content_utils.py Outdated Show resolved Hide resolved
integreat_cms/cms/utils/content_utils.py Outdated Show resolved Hide resolved
tests/cms/views/pages/test_page_model.py Outdated Show resolved Hide resolved
tests/cms/views/pages/test_page_model.py Outdated Show resolved Hide resolved
JoeyStk
JoeyStk commented Jul 4, 2024
View reviewed changes
integreat_cms/cms/forms/custom_content_model_form.py Outdated Show resolved Hide resolved
integreat_cms/cms/forms/custom_content_model_form.py Outdated Show resolved Hide resolved
integreat_cms/cms/utils/content_utils.py Outdated Show resolved Hide resolved
integreat_cms/cms/utils/content_utils.py Outdated Show resolved Hide resolved
david-venhoff
david-venhoff approved these changes Jul 8, 2024
View reviewed changes
Copy link
Member

@david-venhoff david-venhoff left a comment

Choose a reason for hiding this comment

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

Very nice, thanks :)

@JoeyStk JoeyStk marked this pull request as ready for review July 8, 2024 10:18
@JoeyStk
Copy link
Contributor Author

JoeyStk commented Jul 8, 2024

This PR blocks the PR for iCal, as iCal requires the refactored method :)

@JoeyStk JoeyStk changed the title [WIP] Refactor clean_content Refactor clean_content Jul 8, 2024
@JoeyStk JoeyStk added the ❗ prio: medium Should be scheduled in the forseeable future. label Jul 8, 2024
@david-venhoff david-venhoff mentioned this pull request Jul 8, 2024
Open
MizukiTemma
MizukiTemma approved these changes Jul 9, 2024
View reviewed changes
Copy link
Member

@MizukiTemma MizukiTemma left a comment

Choose a reason for hiding this comment

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

Thank you so much 💪 It's sooo clean now 😻

Just don't forget to squash the commits 😉

@JoeyStk JoeyStk force-pushed the enhancement/refactor-custom-content-model-form branch from 2eee903 to 281040e Compare July 9, 2024 14:08
@JoeyStk JoeyStk merged commit cd8fcd1 into develop Jul 10, 2024
5 checks passed
@JoeyStk JoeyStk deleted the enhancement/refactor-custom-content-model-form branch July 10, 2024 11:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@david-venhoff david-venhoff david-venhoff approved these changes

@MizukiTemma MizukiTemma MizukiTemma approved these changes

@ztefanie ztefanie Awaiting requested review from ztefanie

@seluianova seluianova Awaiting requested review from seluianova

@deen13 deen13 Awaiting requested review from deen13

@timobrembeck timobrembeck Awaiting requested review from timobrembeck

Assignees
No one assigned
Labels
❗ prio: medium Should be scheduled in the forseeable future.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Refactor clean_content in custom_content_model_form
3 participants
@JoeyStk @david-venhoff @MizukiTemma