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

Add settings-path option support #63

Open
sobolevn opened this issue Sep 30, 2018 · 9 comments
Open

Add settings-path option support #63

sobolevn opened this issue Sep 30, 2018 · 9 comments
Assignees

Comments

@sobolevn
Copy link
Contributor

sobolevn commented Sep 30, 2018

There's an awesome setting for the isort that I use quite frequently in my setup: settings-path.
It allows to specify the desired configuration path.

Could I please add a support for this option to this library?
I really miss it, and it won't interfere with nay other options.

That's how I think it should work:

  1. If --no-isort-config is passed, then we ignore all other steps and use the default options
    0.1 We can also raise an exception if both --no-isort-config and --isort-settings-path are passed
    0.2 We can just ignore the --isort-settings-path
  2. If this option is passed - then try to read this file. I have two options in case this file does not exist or can not be red:
    1.1 We can raise an exception. All flake8 check will fail with this exception. This will help to indicate about the problem early
    1.2 Just fallback to the config finding behavior in case this file is missing
  3. If no options are passed we should fallback to the config finding logic

@gforcada What do you think?

I personally prefer explicit exceptions. As they indicate about the problem. And not hiding it.

@gforcada
Copy link
Owner

gforcada commented Nov 6, 2018

@sobolevn hi!

Sorry for replying so late! 🙇‍♂️

I'm ok on adding that option, though I would be great if we could get that from isort itself, would that be possible? 🤔

Anyway, with your current proposal:

  • on 0 I would do 0.2 but rather than silently ignore it, log a warning explaining what happened and what will flake8-isort do: i.e. conflicting options are being used, fallback to try to find the config file on its own
  • on 1 I would do, again, a warning, the file could not be found, fallback to try to find the config file

Sounds reasonable? 🤔

@GideonBear
Copy link

Hello @gforcada, is this planned? I can take a shot at implementing this

@gforcada
Copy link
Owner

@GideonBear not from my side, I would welcome a PR though 👍🏾

@GideonBear
Copy link

@GideonBear not from my side, I would welcome a PR though 👍🏾

Alright, I'll take a look, thanks for the quick reply!
Can you assign me to the issue?

@gforcada
Copy link
Owner

@GideonBear there you are, let me know if you need help 😄

@GideonBear
Copy link

@GideonBear there you are, let me know if you need help smile

Will do, thanks!

@GideonBear
Copy link

@gforcada The plugin is broken on master:
TypeError: parse_options() missing 3 required positional arguments: 'option_manager', 'options', and 'args'
It seems to be caused by 860e7c2.
Tests pass, but running flake8 with the plugin installed crashes, maybe it's a good idea to add a test to see if it doesn't crash?

@GideonBear
Copy link

The docs say:

def parse_options(option_manager, options, args):
    pass
# or
def parse_options(options):
    pass

@gforcada
Copy link
Owner

Yes,, a test like that would be great, you are welcome to write it if you have the time and skills 👍🏾

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

No branches or pull requests

3 participants