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

Adusted grammar in release script readme. #1

Merged
merged 2 commits into from
Mar 6, 2018

Conversation

FlXlT
Copy link

@FlXlT FlXlT commented Mar 6, 2018

Adjusted some grammar in the scripts/release/README.md file.

  • Adjusted title to infer that there are more release scripts
  • The first sentence infers that there is one release script. However, it seems that the release process includes two scripts rather than being a script on its own.
  • Officially, and also confirmed on the homepage readme, the npm logo and name are with lowercase letters.
  • Some abbreviations were missing punctuation.

Copy link

@OlivierPMaas OlivierPMaas left a comment

Choose a reason for hiding this comment

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

Please see comment.

1. The **build** script does the heavy lifting (eg checking CI, running automated tests, building Rollup bundles) and then prints instructions for manual verification.
1. The **publish** script then publishes the built artifacts to NPM and pushes to GitHub.
1. The **build** script does the heavy lifting (e.g., checking CI, running automated tests, building Rollup bundles) and then prints instructions for manual verification.
1. The **publish** script then publishes the built artifacts to npm and pushes to GitHub.

Choose a reason for hiding this comment

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

I agree that this current wording ("The release script runs in 2 passes") is ambiguous. I see two situations:

  1. If there is some sort of hidden release-script that uses build and publish, then we should clarify this.
  2. If there is not, then we should write down that "At a high-level, the release-process uses 2 scripts: build and publish."
    I think situation 2 is a lot more likely, and thus propose the above modification.

Copy link
Author

Choose a reason for hiding this comment

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

Excactly, I was thinking of the word process as well (maybe without the '-' ?). However, what if they 'like' using the word 'release script'. For instance, what about the title of the readme then?

Choose a reason for hiding this comment

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

The issue is further worsened by the fact that this is all inside a folder named "scripts." At this point I'm tempted to just add a release.js-file which simply runs build and release consecutively!

I think it's relatively fine for the readme's title to remain the same, even after our proposed changes. But we could change it to "React Release Scripts" instead.

Copy link
Author

Choose a reason for hiding this comment

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

That's a really good idea although I myself am just unsure whether there is some sort of hidden yarn option which might do this already.

Copy link
Author

Choose a reason for hiding this comment

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

Let's just try to change it to 'Scripts' and await their response?

Choose a reason for hiding this comment

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

Yes, let's.

Copy link

@dalderliesten dalderliesten left a comment

Choose a reason for hiding this comment

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

After changing Olivier's comments, I approve.

Copy link

@OlivierPMaas OlivierPMaas left a comment

Choose a reason for hiding this comment

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

Excellent.

@FlXlT FlXlT dismissed OlivierPMaas’s stale review March 6, 2018 17:58

Fixed feedback

@dalderliesten dalderliesten merged commit 0662481 into fix-doc-script Mar 6, 2018
@dalderliesten dalderliesten deleted the adjust-doc branch March 6, 2018 18:03
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.

3 participants