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

docs: mention CONTRIBUTING.md in README #5976

Closed
wants to merge 1 commit into from

Conversation

ykdojo
Copy link
Contributor

@ykdojo ykdojo commented Oct 23, 2024

Test plan

Manually check README.md

Changelog

@codycwiseman
Copy link

As seen on stream.

Good for both humans and AI

@@ -73,6 +73,10 @@ Cody Enterprise can search context from your entire remote codebase using Source
- [Discord](https://discord.gg/s2qDtYGnAE)
- [Twitter (@sourcegraph)](https://twitter.com/sourcegraph)

## Set Up & Contribute

Please take a look at CONTRIBUTING.md for more information about how to set up this project and contribute.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Please take a look at CONTRIBUTING.md for more information about how to set up this project and contribute.
Please take a look at [CONTRIBUTING.md](CONTRIBUTING.md) for more information about how to set up this project and contribute.

Copy link
Contributor

@dominiccooney dominiccooney left a comment

Choose a reason for hiding this comment

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

Looks good, please make it a link.

@ichim-david
Copy link
Contributor

ichim-david commented Oct 23, 2024

@ykdojo, highjacking this pull request a bit because I see that you guys want to bring more awareness to the Contributing docs.
I have a few comments regarding the Contributions docs and their accuracy as I ran into some issues as I tried to use them as guidelines for my first pull request #5874.
Hopefully, they make sense and are useful as community feedback on pain points dealing with the contribution guidelines or the whole process to go from installing locally to opening a pull request and waiting for feedback on it.

Stuff such as:

  1. The docs mention that to test the web dev the sourcegraph server is set to sourcegraph.com
    https://github.com/sourcegraph/cody/blob/main/doc/dev/index.md#use-web-build-for-quick-ui-iteration
    It used to be that value if I visit
    https://sourcegraph.com/github.com/sourcegraph/cody@c9e483df12dc7547dcdb19abece034c42e0f9039/-/blob/web/demo/App.tsx?L17-19
    but nowadays it is set to sourcegraph.sourcegraph.com which is a bummer since I don't have access to it as an open-source contributor.
    https://sourcegraph.com/github.com/sourcegraph/cody/-/blob/web/demo/App.tsx?L14
    EDIT: I wish this was setup with an environment variable which I could add when running the dev in order to avoid having to modify source files. It could be still set by default to s2 but still allow me to easily add s1 when running dev.
  2. asdf install doesn't work, running it gives the warning:
    Install plugins first to be able to install tools
    Here the docs should clarify what tools need to be installed and that you might have them if you look at this file
    https://github.com/sourcegraph/cody/blob/main/.tool-versions
    "Run asdf install (if needed, run asdf plugin add NAME for any missing plugins)" It's not clear what this means and how you find out what plugins you need to add and this is why I mention linking to the .tool-versions file.
  3. There isn't any mention of pnpm biome command which you should run as part of testing since this process runs on CI and will fail if you make formatting changes such as on vscode/package.json.
    I think it should be mentioned here https://github.com/sourcegraph/cody/blob/main/vscode/CONTRIBUTING.md
  4. It's close to impossible to test everything properly as again s2 access is assumed and the CI system fails to run.
    For instance, trying to run integration tests gives this error
    sourcegrapth-integration-tests
    Or another instance where tests don't run due to github actions that need to be upgraded
    unit-test-setup

Besides those issues I've outlined, most of the pull requests merged are usually failing 1 or 2 tests as you can easily see from this screenshot
cody-failing-tests
This makes it hard for me as an open-source contributor to know if the failures of my pull request are due to my code or simply flaky tests that are known to fail and are not addressed.

For a smoother contribution workflow these issues need to be addressed otherwise the task of contributing will always be cumbersome and worth it only for people who know all of these issues and are willing to go around them.

@ykdojo
Copy link
Contributor Author

ykdojo commented Oct 23, 2024

@ichim-david thank you for mentioning these - I'll try & circle back to this.

For making it a link, I realized there are multiple contribution docs now, and there is also a contribution section that I missed previously.

Given these, I'll close this PR for now without merging it, and I'll revisit it when it makes sense.

@ykdojo ykdojo closed this Oct 23, 2024
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