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

Fix index_indicators condition #2631

Closed

Conversation

mlouielu
Copy link
Contributor

This will not match:

pipenv install -e packages/haha-internal

Fix #2494

This will not match:

    pipenv install -e packages/haha-internal
@techalchemy
Copy link
Member

Hey there, thanks for the PR! I believe the direction we want to go on this is to handle parsing index groups with click if possible, as this logic is kind of messy as is. I’m ok with you taking a crack at it if you like.

I could be persuaded to merge a temporary fix in the meantime, but I believe it would have to handle parsing everywhere we are currently doing it with this approach.

@mlouielu
Copy link
Contributor Author

@techalchemy yeah, sure. But previous time #2299 to make --editable a click option end with closed PR, not sure if it is worth to make it an option or not.

@uranusjr
Copy link
Member

@mlouielu Just do it (the editable option too, I think they are good if done correctly).

@techalchemy
Copy link
Member

The editable one didn’t capture the change needed all the way through the codebase as I recall and it would have just broken a lot of things. I’d love to have these as click options, but I want to re-affirm that doing so involves more than just altering the cli. The logic of both of these flows through the entire application and needs to be traced and modified to accommodate those changes.

@mlouielu
Copy link
Contributor Author

@techalchemy @uranusjr OK, I'll open a new PR for this!

@techalchemy
Copy link
Member

closing for now, I have a fix for this-- thanks for the work you put in on it!

@techalchemy techalchemy closed this Sep 2, 2018
@mlouielu
Copy link
Contributor Author

mlouielu commented Sep 4, 2018

@techalchemy can you point me which commit/PR fix this? Thanks!

@techalchemy
Copy link
Member

#2814

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