Skip to content

Conversation

c4llmeco4ch
Copy link
Contributor

As a follow-up to PR #100 . There are still some issues I am working out presently.

The main one is topological sort might need to be reworked in how it receives tests or it may need to have its own tests written up specifically for it, different from the normal format. This is due to topological taking in a slice of edges rather than a slice of ints.

Also, while not a huge issue, Selection sort seems unable to handle a sufficiently large sort so I may need to dig in and rewire the large tests to account for selection sort being slow.

I'll be passing some updates in the near future, but just wanted to get this on the books ASAP.

@c4llmeco4ch
Copy link
Contributor Author

Checking the linter, a lot of the issues seem to stem from general assignment issues. I'll try to tackle those

@c4llmeco4ch
Copy link
Contributor Author

Everything else is a part of topological

@c4llmeco4ch
Copy link
Contributor Author

c4llmeco4ch commented Dec 31, 2019

Gonna sleep on it. The main problem was this sort was written to accommodate multiple input possibilities and has multiple supporting functions to facilitate those options.

Furthermore, because it will require its own special test suite utilizing a graph rather than a list, the whole topological_sort.py is more or less causing problems for the linter (see the note on how topSort(input []Edge) ([]int, [][]int); is unused).

A bandaid fix is to effectively comment out the whole file while I build a suite for it then deal with the remainder of the tests and put TopSort off for a different PR. Up to you on how you want to proceed.

@c4llmeco4ch
Copy link
Contributor Author

Wrapped the whole thing in a giant comment to get things passing. Happy to tackle this however is deemed most professional, timely, and applicable, but figured something passing seemed to be the highest priority.

@cclauss cclauss merged commit 96c609f into TheAlgorithms:master Dec 31, 2019
@c4llmeco4ch c4llmeco4ch deleted the addTests branch January 1, 2020 06:29
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