Skip to content

Conversation

@paolopavan
Copy link
Contributor

This PR addresses issue #855

Copy link
Contributor

@josemduarte josemduarte left a comment

Choose a reason for hiding this comment

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

Thanks! Looks good to me. Only one minor comment

@josemduarte
Copy link
Contributor

@chenzhiw since you opened the original issue #855 , could you have a look at this?

@paolopavan
Copy link
Contributor Author

By the way, I may just miss some pieces or a different way to represent it, does AbstractSequence misses a kind of isCircular property? Would it be good to add?

@paolopavan
Copy link
Contributor Author

paolopavan commented May 8, 2020

@chenzhiw since you opened the original issue #855 , could you have a look at this?

Sure. Note that the very same sequence mentioned in the issue has been added in test for the new feature.
Still I am wondering on how many sequences out there fall into this case. Any opinion?

Copy link
Contributor

@josemduarte josemduarte 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!

@josemduarte
Copy link
Contributor

I will merge this by end of week if there are no other comments.

I'll then produce a new release.

@heuermh
Copy link
Member

heuermh commented May 18, 2020

Thank you @josemduarte @paolopavan, I will review and test this week.

@josemduarte
Copy link
Contributor

@heuermh any comments? or shall we merge?

@sbliven
Copy link
Member

sbliven commented May 29, 2020

Looks good to me. Thanks @paolopavan!

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.

4 participants