Skip to content

Formatting error on PR #1687

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

Closed
yijiezh opened this issue Jul 8, 2020 · 5 comments
Closed

Formatting error on PR #1687

yijiezh opened this issue Jul 8, 2020 · 5 comments

Comments

@yijiezh
Copy link

yijiezh commented Jul 8, 2020

Describe the bug
I was trying to send out a PR for a fix.
#1618
The PR keep failing with formatting error introduced by black and flake8.

black will always format the line into

            return [
                {"Name": er_mean, "Regex": "{}: {}".format(er_mean, float_regex),},
                {"Name": er_max, "Regex": "{}: {}".format(er_max, float_regex),},
            ]

While flake8 will always complain about ',' should be followed by a space.

I tried different approaches but black will always formatting it back to the format above.

Any suggestion?

@nadiaya
Copy link
Contributor

nadiaya commented Jul 10, 2020

The following way the tests would pass:

                {"Name": "episode_reward_mean", "Regex": "episode_reward_mean: (%s)" % float_regex},
                {"Name": "episode_reward_max", "Regex": "episode_reward_max: (%s)" % float_regex},

Any particular reason why you want to change original formatting?

@yijiezh
Copy link
Author

yijiezh commented Jul 13, 2020

This is the original codes, but it didn't get passed.

if toolkit is RLToolkit.COACH:
            return [
                {"Name": "reward-training", "Regex": "^Training>.*Total reward=(.*?),"},
                {"Name": "reward-testing", "Regex": "^Testing>.*Total reward=(.*?),"},
            ]
        if toolkit is RLToolkit.RAY:
            float_regex = "[-+]?[0-9]*[.]?[0-9]+([eE][-+]?[0-9]+)?"  # noqa: W605, E501

            return [
                {
                    "Name": "episode_reward_mean",
                    "Regex": "episode_reward_mean: (%s)" % float_regex,
                },
                {"Name": "episode_reward_max", "Regex": "episode_reward_max: (%s)" % float_regex,},
            ]
        raise ValueError("An unknown RLToolkit enum was passed in. toolkit: {}".format(toolkit))

It seems like black will always format it as float_regex,}, and that will conflicts with flake8.
I tried to reformat it as the way above since I doubt it's due to line length at the beginning.

@chuyang-deng
Copy link
Contributor

Hi, could you go with the black format by running tox -e black-format, and run tox -e flake8. If flake8 is complaining the line is too long, you could add an in-line ignore marker: https://flake8.pycqa.org/en/3.1.1/user/ignoring-errors.html#in-line-ignoring-errors

@davidbrochart
Copy link
Collaborator

@yijiezh did you try @chuyang-deng's suggestion?
Or do you think this issue is not relevant anymore?

@akrishna1995
Copy link
Contributor

@yijiezh I see the changes are merge. The formatting are succeeding in the recent PRs. Closing this issue, please feel free to reopen if there are any more questions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants