Skip to content

Conversation

@purpleP
Copy link
Contributor

@purpleP purpleP commented May 23, 2018

I suppose it's a bug, but I'm not sure, because I don't fully understand from where are all possible configurations can be taken.

@palantirtech
Copy link
Member

Thanks for your interest in palantir/python-language-server, @purpleP! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

@gatesn
Copy link
Contributor

gatesn commented May 23, 2018

Yup, was introduced in this PR: https://github.com/palantir/python-language-server/pull/335/files

Thanks!

@purpleP
Copy link
Contributor Author

purpleP commented May 23, 2018

@gatesn I agree about try expect continue being more pythonic here, but I should add that there's a bigger problem here - code duplication. I can refactor that if someone would explain to me how this supposed to work. Seems like maybe one could change merge_dicts to take variable number of dicts instead of just two and just merge all configs into one.

@gatesn
Copy link
Contributor

gatesn commented May 23, 2018

Yeah, agree this could be refactored to be much nicer. Config providers should be lazily loaded iff they are installed, merging configs should be cached etc. Happy for you to take a look at this if you like? Either way, we should get this bug fix in I think.

@gatesn gatesn merged commit d8eb65f into palantir:develop May 27, 2018
@gatesn gatesn mentioned this pull request May 27, 2018
@purpleP purpleP deleted the fix_config branch May 27, 2018 17:49
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.

3 participants