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

CLN remove unnecessary trailing commas to get ready for new version of black #35925

Closed
4 of 23 tasks
MarcoGorelli opened this issue Aug 27, 2020 · 20 comments · Fixed by #36493
Closed
4 of 23 tasks

CLN remove unnecessary trailing commas to get ready for new version of black #35925

MarcoGorelli opened this issue Aug 27, 2020 · 20 comments · Fixed by #36493

Comments

@MarcoGorelli
Copy link
Member

MarcoGorelli commented Aug 27, 2020

The new version of black is consistent in how it handles the magic trailing commas. So if we upgrade black and apply it, lots of files will be changed. However, the diff needn't be so large if we remove unnecessary trailing commas before upgrading.

E.g. in pandas/core/aggregation.py there is

def reconstruct_func(
    func: Optional[AggFuncType], **kwargs,
) -> Tuple[
    bool, Optional[AggFuncType], Optional[List[str]], Optional[List[int]],
]:

which has an unnecessary trailing comma.

The new version of black would transform this as

def reconstruct_func(
    func: Optional[AggFuncType],
    **kwargs,
) -> Tuple[bool, Optional[AggFuncType], Optional[List[str]], Optional[List[int]],]:

However, if we instead remove the trailing comma and write it as

def reconstruct_func(
    func: Optional[AggFuncType], **kwargs
) -> Tuple[bool, Optional[AggFuncType], Optional[List[str]], Optional[List[int]]]:

then both the current and the new versions of black will be OK with it.

So, PRs to remove some unnecessary trailing commas would be welcome - perhaps keep each PR limited to 5-10 files changed.

Files that (may) need changing are:

  • /asv_bench/benchmarks/arithmetic.py
  • /pandas/core/array_algos/replace.py
  • /doc/make.py
  • /doc/source/conf.py
  • /pandas/core/aggregation.py
  • /pandas/core/algorithms.py
  • /pandas/_vendored/typing_extensions.py
  • /pandas/core/util/numba_.py
  • /pandas/core/sorting.py
  • /pandas/io/formats/latex.py
  • /pandas/core/series.py
  • /pandas/io/formats/format.py
  • /pandas/core/frame.py
  • /pandas/tests/arrays/sparse/test_array.py
  • /pandas/tests/frame/test_analytics.py
  • /pandas/tests/indexes/base_class/test_indexing.py
  • /pandas/tests/indexes/test_common.py
  • /pandas/tests/indexes/test_numeric.py
  • /pandas/tests/io/test_gcs.py
  • /pandas/tests/io/test_parquet.py
  • /pandas/tests/scalar/timestamp/test_constructors.py
  • /pandas/tests/series/test_operators.py
  • /scripts/tests/test_validate_docstrings.py

EDIT

updated list of files which need changing

@MarcoGorelli
Copy link
Member Author

MarcoGorelli commented Aug 27, 2020

See #35930, #35949, #35950 and for examples of how to open a PR for this.

No need to ask for permission to work on this, just choose a few files to work on and open a PR (optionally, you can comment here indicating that you're working on them)

@jorisvandenbossche
Copy link
Member

I think the easiest way to find them is actually to let black (using the latest version) reformat the file(s), and then check in the diff for which ones the trailing comma can be removed (and then run black again)

@jpribyl
Copy link
Contributor

jpribyl commented Aug 28, 2020

I am looking at these right now

  • pandas/core/internals/concat.py
  • pandas/core/internals/managers.py
  • pandas/core/internals/ops.py
  • pandas/core/nanops.py
  • pandas/core/ops/docstrings.py
  • pandas/core/reshape/concat.py
  • pandas/core/reshape/pivot.py
  • pandas/core/reshape/reshape.py

edit: #35956

@MarcoGorelli if it looks good I can open another with some more later today.

@MarcoGorelli
Copy link
Member Author

@jpribyl thanks! Yes, feel free to do so, any help here would be appreciated!

@jpribyl
Copy link
Contributor

jpribyl commented Aug 28, 2020

#35959

with several more files:

  • pandas/core/series.py
  • pandas/core/window/ewm.py
  • pandas/core/window/indexers.py
  • pandas/core/window/numba_.py
  • pandas/core/window/rolling.py
  • pandas/io/formats/css.py
  • pandas/io/formats/format.py
  • pandas/io/orc.py

This was referenced Aug 29, 2020
@metehankutlu
Copy link
Contributor

metehankutlu commented Aug 30, 2020

working on these right now:

  • pandas/tests/test_multilevel.py
  • pandas/tests/test_nanops.py
  • pandas/tests/window/moments/test_moments_consistency_rolling.py
  • pandas/tests/window/moments/test_moments_ewm.py
  • pandas/tests/window/moments/test_moments_rolling.py
  • pandas/tests/window/test_base_indexer.py
  • pandas/tests/window/test_pairwise.py
  • pandas/tests/window/test_rolling.py
  • pandas/tseries/frequencies.py
  • pandas/util/_test_decorators.py

#35996

@metehankutlu
Copy link
Contributor

metehankutlu commented Aug 30, 2020

I'm searching using this regex: ,\n? *\)|,\n? *\] What do you guys think? Is it the correct way?

Edit: ,\n? *(\)|\]|\})

@MarcoGorelli
Copy link
Member Author

I'm searching using this regex: ,\n? *\)|,\n? *\] What do you guys think? Is it the correct way?

Edit: ,\n? *(\)|\]|\})

I would just upgrade black, run it on the chosen files, see what's changed, remove trailing commas, and run black again

@Anshoo-Rajput
Copy link
Contributor

I would just upgrade black, run it on the chosen files, see what's changed, remove trailing commas, and run black again

@MarcoGorelli when I am running black again, it changes file back with trailing commas

@MarcoGorelli
Copy link
Member Author

MarcoGorelli commented Sep 1, 2020

I would just upgrade black, run it on the chosen files, see what's changed, remove trailing commas, and run black again

@MarcoGorelli when I am running black again, it changes file back with trailing commas

That's fine. Two things may happen when running the new version of black:

  • the command is long, and so it splits it into multiple lines, each with a trailing comma;
  • the command is short, and so it puts it all on one line.

It's the second case in which we want to remove the trailing commas, so that both the current and the new version of black would reformat that line in the same way.

From a quick glance it looks like you've done it correctly anyway, will review later today

@tiagohonorato
Copy link
Contributor

Working on these files:

  • pandas/io/sas/sas_xport.py
  • pandas/io/stata.py
  • pandas/tests/arithmetic/test_interval.py
  • pandas/tests/arithmetic/test_numeric.py
  • pandas/tests/arrays/boolean/test_logical.py

@sarthakvk
Copy link

Now Working on

  • pandas/tests/scalar/test_na_scalar.py
  • pandas/tests/scalar/timestamp/test_arithmetic.py
  • pandas/tests/scalar/timestamp/test_constructors.py
  • pandas/tests/series/methods/test_argsort.py
  • pandas/tests/series/methods/test_convert_dtypes.py
  • pandas/tests/series/methods/test_drop_duplicates.py
  • pandas/tests/series/methods/test_interpolate.py
  • pandas/tests/series/methods/test_unstack.py
  • pandas/tests/series/test_cumulative.py
  • pandas/tests/test_algos.py

@xcz011
Copy link
Contributor

xcz011 commented Sep 2, 2020

Working on:

  • pandas/tests/groupby/aggregate/test_numba.py
  • pandas/tests/groupby/test_apply.py
  • pandas/tests/groupby/test_categorical.py
  • pandas/tests/groupby/test_groupby_dropna.py
  • pandas/tests/groupby/test_groupby_subclass.py
  • pandas/tests/groupby/test_size.py
  • pandas/tests/groupby/test_timegrouper.py
  • pandas/tests/groupby/transform/test_numba.py

@sarthakvk
Copy link

sarthakvk commented Sep 2, 2020

Working on these files

  • pandas/tests/io/test_sql.py
  • pandas/tests/io/test_stata.py
  • pandas/tests/plotting/test_frame.py
  • pandas/tests/resample/test_datetime_index.py
  • pandas/tests/reshape/merge/test_merge_index_as_string.py
  • pandas/tests/reshape/test_crosstab.py
  • pandas/tests/reshape/test_get_dummies.py

@satrio-hw
Copy link
Contributor

hello, I'm new to this community.. Now I'm working on these files :

  • pandas/tests/arrays/categorical/test_replace.py
  • pandas/tests/arrays/sparse/test_array.py
  • pandas/tests/arrays/test_array.py
  • pandas/tests/arrays/test_timedeltas.py
  • pandas/tests/base/test_conversion.py
  • pandas/tests/dtypes/test_missing.py
  • pandas/tests/extension/base/methods.py
  • pandas/tests/extension/test_sparse.py
  • pandas/tests/frame/indexing/test_setitem.py

@tiagohonorato
Copy link
Contributor

Working on:

  • pandas/tests/io/pytables/test_timezones.py
  • pandas/tests/io/test_feather.py
  • pandas/tests/io/test_s3.py
  • pandas/tests/io/test_sql.py
  • pandas/tests/io/test_stata.py
  • pandas/tests/plotting/test_frame.py
  • pandas/tests/resample/test_datetime_index.py
  • pandas/tests/reshape/merge/test_merge_index_as_string.py
  • pandas/tests/reshape/test_crosstab.py
  • pandas/tests/reshape/test_get_dummies.py

@cjlynch278
Copy link
Contributor

Working On:
pandas/tests/series/methods/test_interpolate.py
pandas/tests/series/methods/test_unstack.py
pandas/tests/series/test_cumulative.py
pandas/tests/test_algos.py

@Abishek15592
Copy link

Working On:
pandas/tests/scalar/test_na_scalar.py
pandas/tests/scalar/timestamp/test_arithmetic.py
pandas/tests/scalar/timestamp/test_constructors.py

@lacrosse91
Copy link
Contributor

lacrosse91 commented Sep 18, 2020

working on:

  • pandas/tests/groupby/transform/test_numba.py

@lacrosse91
Copy link
Contributor

working on:

  • /pandas/tests/indexes/base_class/test_indexing.py
  • /pandas/tests/indexes/test_common.py
  • /pandas/tests/indexes/test_numeric.py

@jorisvandenbossche jorisvandenbossche modified the milestones: Contributions Welcome, 1.2 Sep 22, 2020
kesmit13 pushed a commit to kesmit13/pandas that referenced this issue Nov 2, 2020
kesmit13 pushed a commit to kesmit13/pandas that referenced this issue Nov 2, 2020
kesmit13 pushed a commit to kesmit13/pandas that referenced this issue Nov 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.