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

Fix missing x-Axis on 1D Timeframe #774

Merged
merged 1 commit into from
Jul 6, 2023

Conversation

Flole998
Copy link
Contributor

@Flole998 Flole998 commented Jul 6, 2023

Unfortunately the fix from #717 caused the Axis to disappear in certain cases (for example when a Timeframe of 1d was used). This fix is better as it goes back to the first attempt and then ignores if the labels are not unique. It is mentioned in the code that usually the first one works alright, so in case we didn't find anything better we should use the first one as a fallback and not the last one as in my original fix.

@Flole998
Copy link
Contributor Author

Flole998 commented Jul 6, 2023

@timmolter @mccartney This is an important fix for us over at openHAB, is there any chance this could be merged and a new release could be made within the next week? I would really appreciate it, thanks a lot for all the effort!

Copy link
Collaborator

@mccartney mccartney left a comment

Choose a reason for hiding this comment

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

It's fine for now as it supposedly fixes some specific problem.

The code gets even more complicated. We'll need to clean it up.

@mccartney mccartney merged commit caaca95 into knowm:develop Jul 6, 2023
1 check passed
@Flole998
Copy link
Contributor Author

Flole998 commented Jul 6, 2023

Thanks a lot!

And yes, but the entire codebase needs cleaning up. Especially all those Print-Statements that are commented out should be based on some global variable to turn them on and off easily.

This function has been "tweaked" many times apparently and it's probably better to understand it and then rewrite it based on what it should do instead of trying to fix it again and again (although I think it's finally "good" now). It might even make sense to split parts of it into a new function.

@mccartney
Copy link
Collaborator

although I think it's finally "good" now)

Famous last words :)

@Flole998
Copy link
Contributor Author

@timmolter Any chance we could get a new release soon? On the last release at least a 24 hour timespan on an axis is broken and causes the axis to disappear (which is a regression), and with this "Hotfix" included that issue would be gone again.

@timmolter
Copy link
Member

timmolter commented Jul 19, 2023

yes, this week sometime. Any other PRs you want to merge @mccartney ?

@mccartney
Copy link
Collaborator

No, thanks. I am good

@timmolter
Copy link
Member

I'll push a release tomorrow.

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.

3 participants