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

Abstract base class for DS #56

Closed

Conversation

abramovd
Copy link

  • Abstract base class for Data structures. Currently used only for data_structures.stack.
  • Mixin for data structures unittests to check that data structure implements required methods (might be useful for new developers)

One TODO:
I don't fully understand either time_complexities method is required or not. I can't see it in a majority of implemented data structures. I added it to base class, but only with pass in its body for now.

If such refactoring approach is ok, I can do the same for other DS classes.

@OmkarPathak
Copy link
Owner

@IanDoarn @Tjstretchalot what do you think, should we use this method for all data structures? I really like the way. Just wanted a second thought from someone.

@Tjstretchalot
Copy link
Contributor

Tjstretchalot commented Aug 29, 2017

I think it belongs in the documentation, personally. It's pretty difficult to find for some algorithms, and it's less obvious we don't have it if it's in the documentation

@OmkarPathak
Copy link
Owner

@Tjstretchalot so should we incorporate the changes or not?

@Tjstretchalot
Copy link
Contributor

I like the base class, I don't like the DS_Class variable and changing the base class for the tests. I'm not sure I see the point in that exactly, since you can accomplish the same thing without using DS_Class and the interface mixin is a lot harder to read

@OmkarPathak
Copy link
Owner

Okay. So sadly @abramovd we have to drop this idea. But thanks for your contribution anyway 👍

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.

None yet

3 participants