-
Notifications
You must be signed in to change notification settings - Fork 176
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
Some clean up in CONTRIBUTING.md and fix broken links & redirects #1038
Some clean up in CONTRIBUTING.md and fix broken links & redirects #1038
Conversation
Also removed [style.css] which does not seem to be used anywhere.
Not sure, but it seems like this test is a noop if we assume that neither Travis nor Appveyor is used.
Docs: fix broken links and redirects
b46ba85
to
59006a9
Compare
Fixes #839 |
(* Check if this is running inside Travis or AppVeyor. *) | ||
let in_travis = | ||
try ignore (Sys.getenv "TRAVIS_COMMIT"); true | ||
with Not_found -> false | ||
in | ||
|
||
let in_appveyor = | ||
try ignore (Sys.getenv "APPVEYOR_REPO_COMMIT"); true | ||
with Not_found -> false | ||
in | ||
|
||
if not (in_travis || in_appveyor) then Lwt.return_true | ||
else Lwt.return Lwt_config.libev_default); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of deleting this, you could check whether the CI
env var is true
or True
, or check whether GITHUB_ACTIONS
is true
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't quite sure of the meaning of this test. Since in_travis
and in_appveyor
would always be false, it's basically a noop. I can do what you propose, and then the meaning of the test would be "if we're in github actions, then check that Lwt_config.libev_default
is true". Is that what we want the test to check (if so, why do we want to test that?)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's probably left-over from special-casing for different CIs and I think we can remove it.
- drop reddit info (deprecated) - add reference to lwt_domain
Thanks a lot for the long overdue fixes |
This PR: