Skip to content

Fix #1818: Whitelist Function1 and Function2 #2480

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

Closed
wants to merge 2 commits into from

Conversation

nicolasstucki
Copy link
Contributor

No description provided.

@DarkDimius
Copy link
Contributor

DarkDimius commented May 19, 2017

LGTM after the tests pass. Curious to know if the cyclic reference error was fixed(and how), or we just compile files in different order now.

@nicolasstucki
Copy link
Contributor Author

This was a bootstrap issue, we did a lot of improvements since it was blacklisted.

@DarkDimius
Copy link
Contributor

@nicolasstucki CI disagrees with you.

@nicolasstucki
Copy link
Contributor Author

@DarkDimius I ran all the CI tests on my machine before sending this PR. I will have to double check :(

@nicolasstucki
Copy link
Contributor Author

@DarkDimius my machine disagrees with CI. Again...

@DarkDimius
Copy link
Contributor

@nicolasstucki, different order of files? One of the reasons whitelist existed before is that it also provided a specific order of files.

@nicolasstucki
Copy link
Contributor Author

I will try adding an order to the files. One that does not depend on the filesystem.

@DarkDimius
Copy link
Contributor

@nicolasstucki I propose to order not by the file name, but instead by class name.
The reason is due to way people report bugs. They rarely post file names.

@smarter
Copy link
Member

smarter commented May 19, 2017

We already have some code to order by filename, but I guess it's not used everywhere: #2193
I'd like to keep ordering things by filename because I have at least one test which relies on it.

@nicolasstucki
Copy link
Contributor Author

Will close this PR for now. This is not a priority.

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