Skip to content

Conversation

xemwebe
Copy link
Contributor

@xemwebe xemwebe commented Oct 15, 2022

This pull request fixes issue #421 by implementing unmap and ReversibleRanged for TimeValue types.

@xemwebe
Copy link
Contributor Author

xemwebe commented Oct 16, 2022

I just realized that for tiny periods (some thousand nanoseconds and below), my solution does not work well, I will fix this during the day.

@xemwebe
Copy link
Contributor Author

xemwebe commented Oct 16, 2022

If have update the PR to work also for small time periods and removed the unnecessary clone()-calls.

@AaronErhardt
Copy link
Member

Thanks looks good overall!

Yet, I'm really not sure whether the master branch is the right place for this. So far, the master branch is only for the current 0.3 stable series and this would be a breaking change, even though most users probably wouldn't notice...
Also, let me know you have any problems with the next-release-devel branch.

@xemwebe
Copy link
Contributor Author

xemwebe commented Oct 17, 2022

I can't see how this changes may break any existing code (basically I just add some functionality). However, if you still prefer to base the changes on the next-release-devel branch, I will just do that, it's no big deal.

@xemwebe xemwebe changed the base branch from master to next-release-devel October 17, 2022 08:32
@xemwebe xemwebe changed the base branch from next-release-devel to master October 17, 2022 08:53
@xemwebe xemwebe mentioned this pull request Oct 17, 2022
@xemwebe
Copy link
Contributor Author

xemwebe commented Oct 17, 2022

Hm, simple rebase does not work since next-release-devel is based on 0.3.2, while master is already on 0.3.4. Apparently, features have been added to master after creating next-release-devel.

Therefore, I have prepared a new PR with the same chnages based on the next-release-devel branch.

@xemwebe xemwebe closed this Oct 17, 2022
@AaronErhardt
Copy link
Member

I can't see how this changes may break any existing code (basically I just add some functionality). However, if you still prefer to base the changes on the next-release-devel branch, I will just do that, it's no big deal.

Honestly, I might have been wrong with this. It looked to me like the TimeValue was public because it's marked as pub but it seems to be a case of an unreachable pub item. If this is correct and the trait is not used in any public interface, this means that this PR actually has no breaking change and would do great on the master branch.

Can you try to remove the pub from the TimeValue trait? If that works, please reopen this PR so the current stable series also profits from this change :)

@xemwebe
Copy link
Contributor Author

xemwebe commented Oct 20, 2022

Can you try to remove the pub from the TimeValue trait? If that works, please reopen this PR so the current stable series also profits from this change :)

That does not seem to work, since TimeValue is used as a constraint in other interfaces that need to be public, i.e. TimeValue needs to be public. If the trait is still hidden in the sense the a plotters users could not implement this trait for their own types the changes might still not be considered to be breaking, but that's arguable.

@AaronErhardt
Copy link
Member

Adding methods to a trait that can't be implemented externally in not a breaking change. I don't see how this could break any existing code.

@xemwebe
Copy link
Contributor Author

xemwebe commented Oct 20, 2022

So what is your suggestion? Re-opening the PR and merge the changes to master as well or wait till the development branch is merged to master?

@AaronErhardt
Copy link
Member

I think, since the work is already done we should merge this into master so this can make it's way into the next release of the current stable series.

@AaronErhardt AaronErhardt reopened this Oct 20, 2022
@AaronErhardt AaronErhardt merged commit 5ccdf3a into plotters-rs:master Oct 20, 2022
@AaronErhardt
Copy link
Member

Thanks!

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