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 VS code settings.json file #42

Merged
merged 4 commits into from
Jul 29, 2022
Merged

Add VS code settings.json file #42

merged 4 commits into from
Jul 29, 2022

Conversation

alexdewar
Copy link
Collaborator

@alexdewar alexdewar commented Jul 29, 2022

So I started out by adding the settings.json file suggested by @dalonsoa, but I've made
a few changes:

Closes #27.

@alexdewar alexdewar requested a review from dalonsoa July 29, 2022 10:19
@davidorme
Copy link
Collaborator

Just to note that my plan is not to use the *.ipynb format for Jupyter notebooks. See:
https://github.com/ImperialCollegeLondon/virtual_rainforest/blob/develop/docs/source/development/jupyter_notebooks.md

I don't know if that changes the settings.

@alexdewar
Copy link
Collaborator Author

Overview of VS code settings: https://code.visualstudio.com/docs/getstarted/settings

Copy link
Collaborator

@dalonsoa dalonsoa left a comment

Choose a reason for hiding this comment

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

The testing options are for VSCode. Just setup.cfg or pyproject.toml is not enough.

],
"python.testing.unittestEnabled": false,
"python.testing.pytestEnabled": true
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be here, otherwise VSCode testing does not know what to use for tests.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It still is set to true! See below

Copy link
Collaborator

Choose a reason for hiding this comment

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

omg

Copy link
Collaborator

@jaideep777 jaideep777 left a comment

Choose a reason for hiding this comment

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

Submitting a sample review.

.vscode/settings.json Outdated Show resolved Hide resolved
@alexdewar
Copy link
Collaborator Author

The testing options are for VSCode. Just setup.cfg or pyproject.toml is not enough.

So I think we can drop the pytest arguments -s tests because if the test directory is set appropriately in e.g. setup.cfg then I think VS code will detect it (and so the test tab will work properly) and if it's only set in settings.json then VS code's test tab will work, but confusingly, it won't work when you run pytest from the command line (or when the CI system runs it).

@dalonsoa
Copy link
Collaborator

Ok, we can leave it like that and change it if it does not work

@jaideep777 jaideep777 self-requested a review July 29, 2022 10:59
As discussed with @dalonsoa.

I've also added a tests folder.
@alexdewar alexdewar requested a review from dalonsoa July 29, 2022 11:04
@@ -1,7 +1,22 @@
{
"python.testing.pytestArgs": [
"."
"python.linting.pylintEnabled": true,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It looks like pylint isn't in the poetry dev dependencies btw. Do we want to enable it or not?

I suggest we either don't use this linter or add it to the dependencies.

@dalonsoa @davidorme

Copy link
Collaborator

Choose a reason for hiding this comment

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

If we want to use it - and it seems we want, right @davidorme - it should be in the dependency list, yes.

@alexdewar
Copy link
Collaborator Author

Just to note that my plan is not to use the *.ipynb format for Jupyter notebooks. See: https://github.com/ImperialCollegeLondon/virtual_rainforest/blob/develop/docs/source/development/jupyter_notebooks.md

I don't know if that changes the settings.

We could remove the setting if it's not needed. It shouldn't break things though!

I leave that decision to the VR dev team. Would you like me to remove it or no? 😄

Copy link
Collaborator

@dalonsoa dalonsoa left a comment

Choose a reason for hiding this comment

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

Keeping aside adding pylint to the dependency list, it looks good to me

@@ -1,7 +1,22 @@
{
"python.testing.pytestArgs": [
"."
"python.linting.pylintEnabled": true,
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we want to use it - and it seems we want, right @davidorme - it should be in the dependency list, yes.

],
"python.testing.unittestEnabled": false,
"python.testing.pytestEnabled": true
Copy link
Collaborator

Choose a reason for hiding this comment

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

omg

@alexdewar alexdewar merged commit fb63eda into develop Jul 29, 2022
@alexdewar alexdewar deleted the vscode_settings branch July 29, 2022 13:23
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.

Common VSCode settings
4 participants