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

Deprecate old term gallery in favor of new "Marketplace". #1259

Merged
merged 2 commits into from
Apr 9, 2021

Conversation

glima
Copy link
Contributor

@glima glima commented Mar 31, 2021

docs/run.md Outdated Show resolved Hide resolved
docs/run.md Show resolved Hide resolved
@glima glima force-pushed the main branch 3 times, most recently from f0115de to 7c4af14 Compare April 2, 2021 23:47
Copy link

@atinmu atinmu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@@ -27,7 +27,14 @@ Follow the [instal](install.md) steps to prepare the source code. Then follow th

### Visual Studio Code

First, click on the Python version at the bottom left and enter the path where the above command was issued. This will point the Code to the Poetry virtual environment.
First, click on the Python version at the bottom left of the editor's
window and enter the path where the above command was issued. You
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can it be put in troubleshooting? It's not met in every setup.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get it. If you do what I added, then VSCode seems to start guessing right every next time you open it, so it is a setup instruction. You move it after we merge, if it pleases you, OK?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The poetry bases on current folder to provide different environments. My understanding is if we get current Python path right, the poetry can make its venv path right. So if provide poetry venv settings here, it may be confusing which Python underlying the poetry.

I'm using poetry with anaconda together. I just need to create an env in anaconda, and poetry will leverage that folder, and no other venv created. It doesn't need to put the venv path into settings.

So I hope this part can be moved to troubleshooting part, let users pick it up based on what problem they meet. If you don't have bandwidth on this part. Can you describe what problem you met, and put the content into an issue? I will update this part later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well I'm not attached to this piece of text, so be it. But we'd better stop practising things and documenting others, like using conda + poetry, no? Are you doing this python-poetry/poetry#1432 ? Try using it with poetry and vscode only and see if it can guess your venv everytime you switch the workplace :) The text is gone now.

@@ -56,6 +63,10 @@ Make sure below settings are in root level of `.vscode/settings.json`.
}
```

While the black formatter does not handle long comments and the like
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about add the explanation in "Other setups"? The link of rewrap can be provided like "Markdown All in One". It doesn't need to mention the black formatter, it's not related to the instruction.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll keep the black remark. It's something it should be doing, instead of letting others do it manually.

@squirrelsc
Copy link
Member

@glima Can we make this PR is simple to its original purpose? We can have different PRs for other topics.

@glima
Copy link
Contributor Author

glima commented Apr 5, 2021

Yeah, new commit mover over to diff PR. Let's not hold this needlessly anymore.

One](https://marketplace.visualstudio.com/items?itemName=yzhang.markdown-all-in-one).
It helps to maintain the table of contents in the documentation.
- While the black formatter does not handle long comments and the like
well (https://github.com/psf/black/issues/1331), you might want to
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@glima I will merge those PRs. I may update the statements later. We represent Microsoft's points. we can provide constructive suggestions what we can. And we consider what value provided to users.

@squirrelsc
Copy link
Member

@glima I tried to fix it from online. It produces another commit. And it's not a good practice to force push your branch. If we want to rebase and merge, can you help on fixup my last commit?

Point out that Rewrap is an option for line wrapping, once black does
not handle it all. Finally, cite a better option to handle multiple
Python venvs in that editor.
@glima
Copy link
Contributor Author

glima commented Apr 8, 2021

Updated with conflicts fixed.

@squirrelsc squirrelsc merged commit ab1dfbf into microsoft:main Apr 9, 2021
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.

None yet

3 participants