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.

Sign up for GitHub

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

Jump to bottom

[docs][scripts] fix broken README.md weblinks and refactor protoc-generator.sh #4027

Merged
merged 1 commit into from
Apr 22, 2021
Merged

[docs][scripts] fix broken README.md weblinks and refactor protoc-generator.sh #4027

merged 1 commit into from
Apr 22, 2021

Conversation

axonasif
Copy link
Member

No description provided.

jankeromnes
jankeromnes reviewed Apr 21, 2021
View reviewed changes
Copy link
Contributor

@jankeromnes jankeromnes left a comment

Choose a reason for hiding this comment

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

Hey, many thanks for your contribution @axonasif!

The README.md link fixes look good to me, but I'm unsure about the protoc-generator.sh refactor.

  • Could you please explain why the refactor was necessary?
  • It might be worth separating these two changes (README.md fixes & protoc refactor) in two separate Pull Requests

README.md
1. [Use a Prefixed URL](https://www.gitpod.io/docs/context-urls/)
1. [Install Browser Extension](https://www.gitpod.io/docs/browser-extension/)
1. [Enable GitLab Integration](https://www.gitpod.io/docs/gitlab-integration/)
1. [Use a Prefixed URL](https://www.gitpod.io/docs/getting-started/#prefixed-url)
Copy link
Contributor

Choose a reason for hiding this comment

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

Praise: Nice touch! (For me, the old link still works, but the one you chose works better here 👍)

Copy link
Member Author

Choose a reason for hiding this comment

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

Feels weird now, yesterday night few of the links were resulting in 404 pages on the gitpod.io side for me, specifically for the getting-started links in README.md. Now all of them seem to be working 😶 Sorry about it.

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@jankeromnes
Copy link
Contributor

Requesting review from @aledbf for the protoc-generator.sh refactor.

@jankeromnes jankeromnes requested a review from aledbf April 21, 2021 09:16
@aledbf
Copy link
Member

aledbf commented Apr 21, 2021

@axonasif the typescript_protoc function change makes sense but not the install_dependencies. What are you trying to change?

@axonasif
Copy link
Member Author

The README.md link fixes look good to me, but I'm unsure about the protoc-generator.sh refactor.

* Could you please explain why the refactor was necessary?

As the title says, the refactoring inside install_dependencies() function is meant to simplify it.
And on line-31(initial) of here, pwd could potentially throw an error because of assigning a value after the local definition but not in a separate instance even though shopt -o errexit is being set from the caller scripts (e.g generate.sh)

* It might be worth separating these two changes (README.md fixes & protoc refactor) in two separate Pull Requests

Will keep that in mind @jankeromnes 😄

@axonasif
Copy link
Member Author

@axonasif the typescript_protoc function change makes sense but not the install_dependencies. What are you trying to change?

Just to simplify the process. In my view using loops instead of repeated commands for the same kind of task seems better.

@aledbf
Copy link
Member

aledbf commented Apr 21, 2021 •
edited
Loading

In my view using loops instead of repeated commands for the same kind of task seems better.

Not in my personal opinion. Please rollback the change in install_dependencies.

Edit: please also squash the commits

@axonasif
Copy link
Member Author

In my view using loops instead of repeated commands for the same kind of task seems better.

Not in my personal opinion. Please rollback the change in install_dependencies.

Okay, understood.

@aledbf
Copy link
Member

aledbf commented Apr 21, 2021

@axonasif please squash the commits and this LGTM

@axonasif
Copy link
Member Author

@axonasif please squash the commits and this LGTM

I'm on it, hold on a bit please.

@axonasif
[docs][script] fix: Broken web-links in README.md; refactor: `protoc-…
24aad7e 
…generator.sh`

[scripts] refactor: Simplify `install_dependencies()` function

Update README.md

Co-authored-by: Jan Keromnes <[email protected]>

revert: Refactoriziation of `install_dependencies()`
@axonasif
Copy link
Member Author

@axonasif please squash the commits and this LGTM

It's done a while back BTW

@aledbf aledbf requested a review from jankeromnes April 21, 2021 11:51
aledbf
aledbf approved these changes Apr 21, 2021
View reviewed changes
Copy link
Member

@aledbf aledbf left a comment

Choose a reason for hiding this comment

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

LGTM

jankeromnes
jankeromnes approved these changes Apr 21, 2021
View reviewed changes
Copy link
Contributor

@jankeromnes jankeromnes 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 to me too, thanks!

@jankeromnes
Copy link
Contributor

jankeromnes commented Apr 21, 2021 •
edited by werft-gitpod-dev-com bot
Loading

/werft run

👍 started the job as gitpod-build-main-fork.2

@csweichel
Copy link
Contributor

csweichel commented Apr 22, 2021 •
edited by werft-gitpod-dev-com bot
Loading

/werft run

👍 started the job as gitpod-build-main-fork.3

@aledbf
Copy link
Member

aledbf commented Apr 22, 2021 •
edited by werft-gitpod-dev-com bot
Loading

/werft run

👍 started the job as gitpod-build-main-fork.4

@aledbf aledbf merged commit 25c7454 into gitpod-io:main Apr 22, 2021
@jankeromnes jankeromnes mentioned this pull request May 10, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jankeromnes jankeromnes jankeromnes approved these changes

@aledbf aledbf aledbf approved these changes

Assignees
No one assigned
Labels
community-contribution
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@axonasif @jankeromnes @aledbf @csweichel