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.

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

Enhancement: add duration format to customapi widget #4549

Merged
merged 2 commits into from
Jan 3, 2025

Conversation

morgendagen
Copy link
Contributor

Proposed change

This PR adds a duration format to the customapi widget. This allows formatting durations (in seconds) in to a more human-readable format, e.g. 1h20m instead of 4,850.

Type of change

  • New service widget
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Documentation only
  • Other (please explain)

Checklist:

  • If applicable, I have added corresponding documentation changes.
  • If applicable, I have reviewed the feature and / or service widget guidelines.
  • I have checked that all code style checks pass using pre-commit hooks and linting checks.
  • If applicable, I have tested my code for new features & regressions on both mobile & desktop devices, using the latest version of major browsers.

@morgendagen morgendagen force-pushed the feature/customapi-duration branch from 3db2762 to 7c34450 Compare January 2, 2025 20:46
@morgendagen morgendagen changed the title Added duration format to the customapi widget. Add duration format to the customapi widget. Jan 2, 2025
@shamoon shamoon changed the title Add duration format to the customapi widget. Enhancement: add duration format to customapi widget Jan 3, 2025
@shamoon shamoon enabled auto-merge (squash) January 3, 2025 00:21
@shamoon
Copy link
Collaborator

shamoon commented Jan 3, 2025

Both the fact that this isn’t linked to a discussion and that it fails linting suggest that you ticked the boxes above but didn’t really read / do them... which is sorta disappointing.

image

I really thought this would make it clear enough, but I guess not.

In the spirit of collaboration, please respect the guidelines for projects to which you would like to contribute.

Please reopen this if / when the guidelines are met.

@shamoon shamoon closed this Jan 3, 2025
auto-merge was automatically disabled January 3, 2025 00:37

Pull request was closed

@morgendagen
Copy link
Contributor Author

Hi,

You are incorrect in your assumption that I did not read, understand, and try to adhere by the rules set forth, and I am sorry that you decided to close this PR without giving me an opportunity to fix the linter issue.

I did not realise that I was committing in a shell where pre-commit hadn't been installed, and assumed that the pre-commit hooks had been executed. As soon as an email popped up in my inbox about a failed workflow, I fixed the issue and was about to push the change when your comment above arrived.

The reason for not opening a feature request / starting a discussion on this PR was based on the word should instead of must combined with the fact that I felt this was such a small change that would have no ongoing maintenance associated with it as it was using an existing feature (common.duration) that is already being utilized elsewhere.

I guess I'll just use my own fork of the project going forward, a project that I have found quite useful.

Have a nice day.

@shamoon
Copy link
Collaborator

shamoon commented Jan 3, 2025

Sorry for being reactionary. I shouldn’t review PRs after work. If you’d like to correct the lint that would be fine.

@shamoon shamoon reopened this Jan 3, 2025
@shamoon shamoon enabled auto-merge (squash) January 3, 2025 03:11
Copy link
Collaborator

@shamoon shamoon left a comment

Choose a reason for hiding this comment

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

Apologies again.

Thanks

@shamoon shamoon merged commit 6f429a6 into gethomepage:dev Jan 3, 2025
@morgendagen
Copy link
Contributor Author

Hi,

My sincere apologies for messing up for not catching the linter issue myself and thank you very much for accepting this PR. Your work on this project is very much appreciated :-)

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