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

Comment and styling cleanup #2163

Merged
merged 6 commits into from
Oct 27, 2020
Merged

Comment and styling cleanup #2163

merged 6 commits into from
Oct 27, 2020

Conversation

M-Davies
Copy link

  • Added comments to groovy files
  • Ran a markdown linter over our docs
  • Ran a spellchecker over both groovy and markdown files

Fixes: #2110
Signed-off-by: Morgan Davies [email protected]

@M-Davies M-Davies added enhancement Issues that enhance the code or documentation of the repo in any way documentation Issues that request updates to our documentation handy/helpful/gives ease labels Oct 20, 2020
@M-Davies M-Davies requested review from karianna, sxa and gdams October 20, 2020 14:20
@adoptopenjdk-github-bot
Copy link
Contributor

🟠 PR TESTER RESULT 🟠

❎ Some pipelines failed or the job was aborted! ❎
See the pipeline-build-check below for more information...

@karianna karianna added this to the October 2020 milestone Oct 20, 2020
karianna
karianna reviewed Oct 20, 2020
View reviewed changes
RELEASING.md Outdated Show resolved Hide resolved
@adoptopenjdk-github-bot
Copy link
Contributor

🟠 PR TESTER RESULT 🟠

❎ Some pipelines failed or the job was aborted! ❎
See the pipeline-build-check below for more information...

Comment and styling cleanup
d01a670 
* Added comments to groovy files
* Ran a markdown linter over our docs
* Ran a spellchecker over both groovy and markdown files

Signed-off-by: Morgan Davies <[email protected]>
@M-Davies M-Davies requested a review from karianna October 22, 2020 10:44
@adoptopenjdk-github-bot
Copy link
Contributor

🟠 PR TESTER RESULT 🟠

❎ Some pipelines failed or the job was aborted! ❎
See the pipeline-build-check below for more information...

karianna
karianna approved these changes Oct 22, 2020
View reviewed changes
Copy link
Contributor

@karianna karianna left a comment

Choose a reason for hiding this comment

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

LGTM

@karianna
Copy link
Contributor

You've got a merge conflict now

@adoptopenjdk-github-bot
Copy link
Contributor

🟠 PR TESTER RESULT 🟠

❎ Some pipelines failed or the job was aborted! ❎
See the pipeline-build-check below for more information...

austin0
austin0 suggested changes Oct 26, 2020
View reviewed changes
Copy link
Contributor

@austin0 austin0 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, thanks for doing this.

pipelines/build/common/build_base_file.groovy Outdated Show resolved Hide resolved
pipelines/build/common/build_base_file.groovy Outdated Show resolved Hide resolved
pipelines/build/common/config_regeneration.groovy Outdated Show resolved Hide resolved
pipelines/build/common/openjdk_build_pipeline.groovy Outdated Show resolved Hide resolved
pipelines/build/common/openjdk_build_pipeline.groovy Outdated Show resolved Hide resolved
sxa
sxa reviewed Oct 26, 2020
View reviewed changes
FAQ.md Show resolved Hide resolved
sxa
sxa reviewed Oct 26, 2020
View reviewed changes
FAQ.md Show resolved Hide resolved
sxa
sxa reviewed Oct 26, 2020
View reviewed changes
README.md Show resolved Hide resolved
sxa
sxa reviewed Oct 26, 2020
View reviewed changes
README.md Outdated Show resolved Hide resolved
sxa
sxa requested changes Oct 26, 2020
View reviewed changes
Copy link
Member

@sxa sxa left a comment

Choose a reason for hiding this comment

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

Lots of nits, several just grammar. Thanks for these changes.

It has reminded me that I need to revisit the docs on makejdk-any-platform.sh (including jep319) and that I've changed the github team names recently - will update those in a separate PR (After this one has gone in so that I don't cause you any conflicts!)

pipelines/build/common/build_base_file.groovy Show resolved Hide resolved
pipelines/build/common/build_base_file.groovy Show resolved Hide resolved
pipelines/build/common/config_regeneration.groovy Show resolved Hide resolved
pipelines/build/common/openjdk_build_pipeline.groovy Show resolved Hide resolved
pipelines/build/common/openjdk_build_pipeline.groovy Show resolved Hide resolved
pipelines/build/common/openjdk_build_pipeline.groovy Outdated Show resolved Hide resolved
pipelines/build/common/openjdk_build_pipeline.groovy Show resolved Hide resolved
pipelines/build/common/openjdk_build_pipeline.groovy Outdated Show resolved Hide resolved
pipelines/build/common/openjdk_build_pipeline.groovy Outdated Show resolved Hide resolved
FAQ.md Show resolved Hide resolved
@M-Davies
Copy link
Author

Thanks for all the reviews! Looks like I missed quite a bit out 😆

@sxa
Applied suggestions from code review
4bc44a7 
Thanks @sxa for providing them. It's a lot easier to use suggestions over comments

Co-authored-by: Stewart X Addison <[email protected]>
@karianna karianna requested a review from austin0 October 26, 2020 13:03
@adoptopenjdk-github-bot
Copy link
Contributor

 PR TESTER RESULT 

❎ Some pipelines failed or the job was aborted! ❎
See the pipeline-build-check below for more information...

Morgan Davies added 2 commits October 26, 2020 17:05
austin0
austin0 approved these changes Oct 26, 2020
View reviewed changes
Copy link
Contributor

@austin0 austin0 left a comment

Choose a reason for hiding this comment

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

LGTM

sxa
sxa approved these changes Oct 26, 2020
View reviewed changes
Copy link
Member

@sxa sxa left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM!

(And I hadn't seen that suggestion that When a committer comments with "LGTM" they specifically mean “I’ve looked at this patch thoroughly and take as much responsibility as if I wrote it myself”. before I reviewed this PR)!

Thanks for putting the time into making all these changes - really appreciate it!

@adoptopenjdk-github-bot
Copy link
Contributor

 PR TESTER RESULT 

❎ Some pipelines failed or the job was aborted! ❎
See the pipeline-build-check below for more information...

@karianna
Copy link
Contributor

@M-Davies failing tests on the Groovy...

Address groovy compile issues
4190fc1 
* Also make weekly test list an empty list
@M-Davies M-Davies merged commit 77389f9 into adoptium:master Oct 27, 2020
@adoptopenjdk-github-bot
Copy link
Contributor

 PR TESTER RESULT 

❎ Some pipelines failed or the job was aborted! ❎
See the pipeline-build-check below for more information...

1 similar comment
@adoptopenjdk-github-bot
Copy link
Contributor

 PR TESTER RESULT 

❎ Some pipelines failed or the job was aborted! ❎
See the pipeline-build-check below for more information...

@M-Davies M-Davies deleted the script_comments branch October 27, 2020 13:30
@sxa sxa mentioned this pull request Oct 30, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@karianna karianna karianna approved these changes

@sxa sxa sxa approved these changes

@austin0 austin0 austin0 approved these changes

@gdams gdams Awaiting requested review from gdams

Assignees
No one assigned
Labels
documentation Issues that request updates to our documentation enhancement Issues that enhance the code or documentation of the repo in any way
Projects
None yet
Milestone
October 2020
Development

Successfully merging this pull request may close these issues.

Add comments to build-scripts
5 participants
@M-Davies @adoptopenjdk-github-bot @karianna @sxa @austin0