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

Simplifies code contributions by automating the dev setup #11300

Merged
merged 1 commit into from
Nov 10, 2019
Merged

Simplifies code contributions by automating the dev setup #11300

merged 1 commit into from
Nov 10, 2019

Conversation

nisarhassan12
Copy link
Contributor

Hi! there, 🙂

this PR adds support for Gitpod, a free online tool that lets you automate your dev environment, eases the hurdles for contributions and generally working on GitHub projects. It allows anyone to get a ready-to-code dev environment for any branch, issue and pull request with a single click.
In the case of this project, it will launch a ready to code workspace with the dependencies installed and the web server running so that contributors can start hacking around with the project without wasting any precious time on the development setup.

You can give it a try on my fork of the repo: https://gitpod.io/#https://github.com/nisarhassan12/pdf.js

image

It seems to work well! Please let me know if there is anything that can be improved. I would love to help :)

@nisarhassan12 nisarhassan12 changed the title Configures gitpod Simplifies code contributions by automating the dev setup Nov 1, 2019
@Snuffleupagus
Copy link
Collaborator

It allows anyone to get a ready-to-code dev environment for any branch, issue and pull request with a single click.

Would this work-flow actually support running any of the PDF.js test-suits, or even running lint, since making it easier for people to submit completely untested PRs probably isn't such a great idea!?

@timvandermeij
Copy link
Contributor

timvandermeij commented Nov 2, 2019

Yes, running lint and unit tests would be a very good idea. Ideally you can also run the regression tests though, but I don't know if that is possible in the workflow; running lint and unit tests should already catch a few issues depending on which code is touched.

@Snuffleupagus
Copy link
Collaborator

Snuffleupagus commented Nov 2, 2019

Ideally you can also run the regression tests though, but I don't know if that is possible in the workflow

Unless it's possible to follow the entire work-flow and run all test-suite (including reference tests) mentioned in https://github.com/mozilla/pdf.js/wiki/Contributing, then advertising this in the README seems like it might cause some unnecessary friction for a would be contributor.

For example: Let's assume that we add this and someone uses it to submit a patch touching e.g. core/ code, and the patch subsequently needs revising because it causes test failures.
At this point, it's quite likely that a reviewer will suggest running tests locally while developing the patch, pointing to https://github.com/mozilla/pdf.js/wiki/Contributing#4-run-lint-and-testing, to which the contributor may then reply:
But, I just followed the "Online setup with a single click" section from the README and {insert test-suite here} won't run there!

That sort of situation doesn't seem great at all, at least as far as I'm concerned.

@nisarhassan12
Copy link
Contributor Author

Thanks! Both linting and the test suite seem to work well 🙂 .

image
image

With this workflow, contributors don't have to generate references images Gitpod automatically does that.

Both running the linting and the test suite is same as it is in local setup i.e gulp lint for linting and gulp test for running the test suite both can be run via the default built-in terminal.

For the server port once the test suite is started contributors can view it from accessing port 6080 from the ports tab in the bottom right corner:
image

Copy link
Collaborator

@Snuffleupagus Snuffleupagus left a comment

Choose a reason for hiding this comment

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

Please note all patches much contain a good commit message that describes exactly what's being changed and why the change is made; hence something like e.g. "Configures gitpod" is not at all useful (even more so when using the Git command line).

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@nisarhassan12
Copy link
Contributor Author

Thanks! @Snuffleupagus I have made the changes and updated the branch.

@Snuffleupagus
Copy link
Collaborator

Snuffleupagus commented Nov 4, 2019

With this workflow, contributors don't have to generate references images Gitpod automatically does that.

At what point is this actually done?
Because as is clearly outlined in https://github.com/mozilla/pdf.js/wiki/Contributing#4-run-lint-and-testing, this step needs to happen before any changes have been made in order for the reference images to be correct.

@nisarhassan12
Copy link
Contributor Author

nisarhassan12 commented Nov 4, 2019

With this workflow, contributors don't have to generate references images Gitpod automatically does that.

At what point is this actually done?
Because as is clearly outlined in https://github.com/mozilla/pdf.js/wiki/Contributing#4-run-lint-and-testing, this step needs to happen before any changes have been made in order for the reference images to be correct.

Oops! Sorry! I was absolutely wrong 😞 I meant to say that contributors don't have to run this command cp test/resources/browser_manifests/browser_manifest.json.example test/resources/browser_manifests/browser_manifest.json which is not actually generating the reference images . The contributors can generate references images as it is in the local setup by running gulp makeref before they make any changes.

@timvandermeij timvandermeij merged commit c2bd3a0 into mozilla:master Nov 10, 2019
@timvandermeij
Copy link
Contributor

Let's give this a try. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants