Skip to content

FIXME: Move format methods from Layout to utils #1835

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Tschuppi81
Copy link
Contributor

@Tschuppi81 Tschuppi81 commented Jun 2, 2025

Layout: Make formatter methods available in utils

With the formatters as utility functions it is not needed to initiate a Layout in order to use the formatters.

TYPE: Feature
LINK: None

Copy link

codecov bot commented Jun 2, 2025

Codecov Report

Attention: Patch coverage is 95.74468% with 2 lines in your changes missing coverage. Please review.

Project coverage is 87.60%. Comparing base (a4e1f67) to head (0c2e1ca).
Report is 6 commits behind head on master.

Files with missing lines Patch % Lines
src/onegov/core/request.py 95.23% 2 Missing ⚠️
Additional details and impacted files
Files with missing lines Coverage Δ
src/onegov/core/layout.py 98.61% <100.00%> (+1.44%) ⬆️
src/onegov/org/forms/newsletter.py 87.22% <100.00%> (-0.28%) ⬇️
src/onegov/swissvotes/layouts/default.py 96.61% <100.00%> (ø)
src/onegov/core/request.py 97.59% <95.23%> (-0.36%) ⬇️

... and 15 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a4e1f67...0c2e1ca. Read the comment docs.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Tschuppi81 Tschuppi81 requested a review from Daverball June 2, 2025 11:32
Copy link
Member

Choose a reason for hiding this comment

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

Since we're always retrieving the locale from request it might be better to make these methods on CoreRequest. That way we can also put the date formats on CoreRequest and keep the API the same as for the methods on Layout. That way we also easily can change the date formats, without having to modify each call site.

@Tschuppi81 Tschuppi81 force-pushed the feature/fixme-move-format-methods-from-layout-to-utils branch from 720069c to 6cdb226 Compare June 3, 2025 06:08
@Tschuppi81 Tschuppi81 force-pushed the feature/fixme-move-format-methods-from-layout-to-utils branch from 6cdb226 to ad9e17a Compare June 3, 2025 06:09
return babel.dates.format_datetime(dt, format=fmt, locale=locale)
else:
return babel.dates.format_date(dt, format=fmt, locale=locale)
fmt = getattr(self, f'{format}_format', format)
Copy link
Member

Choose a reason for hiding this comment

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

I would move this part to CoreRequest as well, we should never manually specify a fmt string, it should always be one of our pre-configured format options. If one of the applications overwrites the formats in their specific layout class, you would need to instead overwrite them in their specific request class, but I'm not sure if we do overwrite them anywhere.

Comment on lines +180 to +182
@pytest.fixture(scope='function')
def org_app(request):
yield create_org_app(request, use_elasticsearch=False)
Copy link
Member

Choose a reason for hiding this comment

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

Don't use OrgApp in core tests, the tests here are supposed to pass with a CoreRequest without any org specific features, so you should create a basic Framework instance here like e.g. maildir_app above.

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.

2 participants