Skip to content

Correct path for README files search in setup#225

Merged
lasofivec merged 7 commits intodevelfrom
Issue224_setup
Nov 5, 2019
Merged

Correct path for README files search in setup#225
lasofivec merged 7 commits intodevelfrom
Issue224_setup

Conversation

@munechika-koyo
Copy link
Copy Markdown
Collaborator

@munechika-koyo munechika-koyo commented Oct 30, 2019

I changed a little code to get rid of bug.
I would appreciate it if you could make sure it is fine and merge it to master branch.

Laura's comments
This pull requests could be divided into two:

@pep8speaks
Copy link
Copy Markdown

pep8speaks commented Oct 30, 2019

Hello @munechika-koyo! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-11-05 16:51:59 UTC

@Didou09 Didou09 self-requested a review October 31, 2019 07:36
@Didou09 Didou09 self-assigned this Oct 31, 2019
@lasofivec lasofivec changed the base branch from master to devel October 31, 2019 15:42
@lasofivec
Copy link
Copy Markdown
Collaborator

lasofivec commented Oct 31, 2019

Hi Koyo, thanks for the PR!
Is this solving issue #224 ?

A couple of remarks:

  • please always do the pull requests to devel. The master branch will be only changed around twice per year to maintain the stability of the library.
  • If you see the comments above, you will see a automatic bot called pep8-speaks that comments on your coding style. I am aware that here it tells you a remark from code that you didn't actually code, and this is due to the fact that we didn't have this tool before and that there are still some areas where the coding style is still not perfect. But just as an exercise: would you mind correcting these errors. Here, pep8-speaks found the following issues:

In the file tofu/geom/utils.py:
Line 629:25: E231 missing whitespace after ':'

if we see the line 629:

'A2': {'Exp':_ExpITER,

it should be changed to:

'A2': {'Exp': _ExpITER,

(notice the withespace after the :)

  • I updated the documentation: see mostly the contributing section. I precisely added some comments on the devel/master comments and on the pep8-speaks

@lasofivec lasofivec self-requested a review October 31, 2019 15:57
Comment thread tofu/geom/utils.py Outdated
_dconfig = {'A1': {'Exp':_ExpWest,
'Ves': ['V1']},
'A2': {'Exp':'ITER',
'A2': {'Exp':_ExpITER,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

this is better ! thanks !

@lasofivec
Copy link
Copy Markdown
Collaborator

Last but not least, since this PR is also introducing the new geometry, could you please describe in the original post what's new on the geometry ? I added the name but some comments could be great.

@lasofivec
Copy link
Copy Markdown
Collaborator

I opened issue #227

Comment thread setup.py
@lasofivec lasofivec changed the title Issue224 setup Solves #224 and new ITER geometry Nov 4, 2019
@lasofivec
Copy link
Copy Markdown
Collaborator

@Didou09 @munechika-koyo since now there is a branch specific to the new geometry addition Issue220_NewITERConfig I feel like this PR should only include the changes to the setup.py.
Event if it will be a small change, I think is good practice to separate the different changes on different PRs.
What do you think ? I can make the changes and close the PR if you argee

@Didou09
Copy link
Copy Markdown
Member

Didou09 commented Nov 5, 2019

I totally agree, let's separate into elementary changes, it's way cleaner

@munechika-koyo
Copy link
Copy Markdown
Collaborator Author

I also agree with @lasofivec
That makes sense.

@munechika-koyo
Copy link
Copy Markdown
Collaborator Author

I raised the new PR #245.
Is this fine?

@lasofivec
Copy link
Copy Markdown
Collaborator

Yes thank you Koyo !!
I'll correct this branch and merge it.

@lasofivec lasofivec changed the title Solves #224 and new ITER geometry Correct path for README files search in setup Nov 5, 2019
@lasofivec lasofivec merged commit b8655b1 into devel Nov 5, 2019
@lasofivec lasofivec deleted the Issue224_setup branch November 5, 2019 16:49
@Didou09 Didou09 mentioned this pull request Nov 20, 2019
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.

4 participants