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

bpo-33702: Add some missing links in production lists and do a little polish #7259

Merged
merged 7 commits into from
Jul 7, 2018

Conversation

andresdelfino
Copy link
Contributor

@andresdelfino andresdelfino commented May 30, 2018

@andresdelfino andresdelfino changed the title bpo-33702: Add some missings links in production lists and a little polish bpo-33702: Add some missing links in production lists and do a little polish Jun 11, 2018
parameter_list_starargs: "*" [`parameter`] ("," `defparameter`)* ["," ["**" `parameter` [","]]]
: | "**" `parameter` [","]
parameter_list: `defparameter` ("," `defparameter`)*
: ["," [`parameter_list_starargs`]] | `parameter_list_starargs`
Copy link
Member

Choose a reason for hiding this comment

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

I think it is better to left this and the following production lists as is.

@@ -816,7 +816,7 @@ or list). Slicings may be used as expressions or as targets in assignment or
slicing: `primary` "[" `slice_list` "]"
slice_list: `slice_item` ("," `slice_item`)* [","]
slice_item: `expression` | `proper_slice`
proper_slice: [`lower_bound`] ":" [`upper_bound`] [ ":" [`stride`] ]
proper_slice: [`lower_bound`] ":" [`upper_bound`] [":" [`stride`]]
Copy link
Member

Choose a reason for hiding this comment

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

I think a space between ]'s increase readability. And the space after [ was added for symmetry.

@@ -862,7 +862,7 @@ series of :term:`arguments <argument>`:
.. productionlist::
call: `primary` "(" [`argument_list` [","] | `comprehension`] ")"
argument_list: `positional_arguments` ["," `starred_and_keywords`]
: ["," `keywords_arguments`]
: ["," `keywords_arguments`]
Copy link
Member

Choose a reason for hiding this comment

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

Left it as is. Extra spaces help to see a difference between the continuation and the alternation.

@@ -621,7 +621,7 @@ for the contents of the string is:
f_string: (`literal_char` | "{{" | "}}" | `replacement_field`)*
replacement_field: "{" `f_expression` ["!" `conversion`] [":" `format_spec`] "}"
f_expression: (`conditional_expression` | "*" `or_expr`)
: ("," `conditional_expression` | "," "*" `or_expr`)* [","]
: ("," `conditional_expression` | "," "*" `or_expr`)* [","]
Copy link
Member

Choose a reason for hiding this comment

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

Same as above.

: ( "," `identifier` ["as" `name`] )*
: | "from" `relative_module` "import" "(" `identifier` ["as" `name`]
: ( "," `identifier` ["as" `name`] )* [","] ")"
import_stmt: "import" `module` ["as" `identifier`] ("," `module` ["as" `identifier`])*
Copy link
Member

Choose a reason for hiding this comment

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

Why name was replaced by identifier?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because, when I added a link to name in future statements, it linked to the name in import statements. That's why I replaced name in both places to "identifier". Perhaps it can be solved in other way?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see, the problem was that name was defined in two places. Then using an identifier LGTM. In the Grammar file the same term is used for the names before and after 'as' in import_from.

decorators: `decorator`+
decorator: "@" `dotted_name` ["(" [`argument_list` [","]] ")"] NEWLINE
dotted_name: `identifier` ("." `identifier`)*
parameter_list: `defparameter` ("," `defparameter`)* ["," [`parameter_list_starargs`]]
: | `parameter_list_starargs`
parameter_list_starargs: "*" [`parameter`] ("," `defparameter`)* ["," ["**" `parameter` [","]]]
: | "**" `parameter` [","]
parameter_list_starargs: "*" [`parameter`] ("," `defparameter`)*
Copy link
Member

Choose a reason for hiding this comment

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

It is better to break a line before | than at arbitrary place. I think it is better to keep the existing formatting. It is not much longer that the previous production list.

: ( "," `identifier` ["as" `name`] )*
: | "from" `relative_module` "import" "(" `identifier` ["as" `name`]
: ( "," `identifier` ["as" `name`] )* [","] ")"
import_stmt: "import" `module` ["as" `identifier`] ("," `module` ["as" `identifier`])*
Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see, the problem was that name was defined in two places. Then using an identifier LGTM. In the Grammar file the same term is used for the names before and after 'as' in import_from.

@miss-islington
Copy link
Contributor

Thanks @andresdelfino for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.6, 3.7.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 7, 2018
… polish (pythonGH-7259)

(cherry picked from commit caccca7)

Co-authored-by: Andrés Delfino <adelfino@gmail.com>
@bedevere-bot
Copy link

GH-8169 is a backport of this pull request to the 3.7 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 7, 2018
… polish (pythonGH-7259)

(cherry picked from commit caccca7)

Co-authored-by: Andrés Delfino <adelfino@gmail.com>
@bedevere-bot
Copy link

GH-8170 is a backport of this pull request to the 3.6 branch.

@andresdelfino andresdelfino deleted the add-missing-links branch July 7, 2018 21:06
miss-islington added a commit that referenced this pull request Jul 7, 2018
… polish (GH-7259)

(cherry picked from commit caccca7)

Co-authored-by: Andrés Delfino <adelfino@gmail.com>
miss-islington added a commit that referenced this pull request Jul 7, 2018
… polish (GH-7259)

(cherry picked from commit caccca7)

Co-authored-by: Andrés Delfino <adelfino@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation in the Doc dir skip news
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants