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

README.rst:grammer fix (missing word) #2047

Merged
merged 2 commits into from Jan 15, 2019
Merged

README.rst:grammer fix (missing word) #2047

merged 2 commits into from Jan 15, 2019

Conversation

andy5995
Copy link
Contributor

I replaced a missing word and suggested a minor change

[skip ci]

I replaced a missing word and suggested a minor change

[skip ci]
Copy link
Member

@b4n b4n left a comment

Choose a reason for hiding this comment

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

LGTM

@elextr
Copy link
Member

elextr commented Jan 14, 2019

@andy5995 but what word did you find missing? Your committ only makes a change, did you miss something?

@b4n
Copy link
Member

b4n commented Jan 14, 2019

@elextr The sentence was "There a few command line options.". I'd agree with @andy5995 that there's at least a missing is, and that the reformulation is better as well.

@elextr
Copy link
Member

elextr commented Jan 14, 2019

@b4n, ok, I missed the "are" that wasn't there (its "are a few" not "is a few" because the "are" refers to the multiple "few", not the singular "a", damn English) 😁

I was checking if @andy5995 left something out because this doesn't sound right, "several" implies a low number.

Now that you made me count them, actually Geany has 21 options, in idiomatic English that is neither "a few" or "several". In fact it is more like"quite a few" though of course it is indeed "only a few" compared to GCC (damn English) 😁

@andy5995 maybe combine the sentences into "For the command line options see the ...." and avoid the numerative ambiguity altogether.

@elextr
Copy link
Member

elextr commented Jan 14, 2019

LGBI, sadly Travis doesn't build the manuals AFAICK. I guess as soon as @b4n indicates he still approves it can be merged and the nightly manual build will check it.

Copy link
Member

@b4n b4n left a comment

Choose a reason for hiding this comment

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

LGBI. Could use squashing when merging.

@b4n
Copy link
Member

b4n commented Jan 15, 2019

@elextr I guess if Travis didn't run it's because of the [skip ci] annotations in the commit messages ;)

@elextr
Copy link
Member

elextr commented Jan 15, 2019

@b4n what I mean is that even if it did run we can't see the manual, I guess it does check rst2html doesn't object too much, but we have no way of checking it looks right from CI.

IIUC Travis artefact uploading isn't available on PR builds, so we have to wait until after merge when the nightly manual is built and uploaded to geany.org.

So here goes this one :)

@elextr elextr merged commit c825b58 into geany:master Jan 15, 2019
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.

None yet

3 participants