Fix CircleCI pylint, esline and flake8 failed checks #110

Bachibouzouk · 2019-01-18T10:53:57Z

Dash Bio pull request

Addressing the eslint errors for the .react.js files described in issue #109 and #114
Addressing the pylint and flake8 errors for the .py files described in issue #111

closes #109 , closes #111

Before asking for a review

I have gone through the code review checklist

PR merging checklist

Please make sure you have done these things before asking for approval to finally merge.

DDS Deployment

Get the developers user credentials from a dash-bio team member.
Checkout the master branch of the dash-bio repository, make sure you have the latest master (git pull).
Check remotes associated to your repository: git remote -v, make sure you have the dash-bio one: https://dash-gallery.plotly.host/GIT/dash-bio.
If you don't have the dash-bio remote add it with https, ssh will not work: git remote add dash-bio https://dash-gallery.plotly.host/GIT/dash-bio.
You will be asked to enter the developers user credentials, have them ready.
Push local master to the dds dash-bio master: git push dash-bio master.

DDS Deployment Debugging

If running git push dash-bio master you may encounter the following issue:

remote: User <username> does not have permissions to run git-hook on dash-bio, or dash-bio does not exist
remote: Access denied
To https://dash-gallery.plotly.host/GIT/dash-bio
 ! [remote rejected] master -> master (pre-receive hook declined)
error: failed to push some refs to 'https://dash-gallery.plotly.host/GIT/dash-bio'

This may be because the git credentials are saved on the client-side. In order to solve this, do the following:

Windows

Go to: Control Panel -> User Accounts -> Manage your credentials -> Windows Credentials -> Under Generic Credentials find: git:https://dash-gallery.plotly.host -> Remove credential OR edit username/password to provided account.

OS X

Go to: Keychain Access -> Search for dash-gallery.plotly.host -> Change "Account" to "developers" -> Click on "show password" -> Change the password to the one provided -> Click "Save Changes".

Bachibouzouk

@shammamah , @VeraZab I addressed almost all the eslint errors, I commented some of my fixes which should be double checked (I looked at the demo app locally and couldn't find differences before and after which should be a good sign)

Bachibouzouk · 2019-01-18T11:53:38Z

src/lib/components/Circos.react.js

@@ -26,7 +26,7 @@ export default class Circos extends Component {
    * Used to set a click or hover event on tracks/layout that will show annotations for the circos grpah. 


Address the improper undefined usage

Bachibouzouk · 2019-01-18T11:56:20Z

src/lib/components/Circos.react.js

        var condColor = configApply.color.conditional
        configApply.color = d => {
+          let returnedColor;
          for (var i = 0; i < condColor.value.length; i++) {
            if (d[condColor.end] - d[condColor.start] > condColor.value[i]) {


Fix Expected to return a value at the end of arrow function. (consistent-return)

Bachibouzouk · 2019-01-18T11:57:47Z

src/lib/components/Ideogram.react.js

@@ -192,13 +192,14 @@ export default class Ideogram extends Component {

        if (this.props.setProps) {
            this.tooltipData = document.getElementById('_ideogramTooltip').innerHTML;


Fix Expected an assignment or function call and instead saw an expression. (no-unused-expressions)

Bachibouzouk · 2019-01-18T11:58:29Z

src/lib/components/Speck.react.js

@@ -115,6 +114,7 @@ export default class Speck extends Component {
 	} 

 	// finally apply the user-supplied view parameters


@shammamah : this one I am not sure how to address...

It's looking for a length prop in view, which doesn't make sense because it's a PropTypes.shape (i.e., object)... I have to find some way to check how many properties there are.

It's looking for a length prop in view, which doesn't make sense because it's a PropTypes.shape (i.e., object)... I have to find some way to check how many properties there are.

@shammamah : could you find out the way to check how many properties there are? I created the issue as you asked me on slack -> #114

Bachibouzouk · 2019-01-18T11:59:08Z

src/lib/components/Speck.react.js

 	// add event listeners
-	const interactionHandler = new speckInteractions(this, renderer, container); 
-


@shammamah : this one too I am not sure how to address

Yeah, I guess this is kind of a weird thing to do. If you look at the code here: https://github.com/shammamah/speck/blob/master/src/interactions.js you'll see that there is a function for the module.exports that doesn't really return anything; it just performs some actions like attaching event handlers to the mouse, scroll wheel, etc.

@shammamah : could you indicate to me which properties on the app this line has an effect on? This would help me check that I haven't break anything :)

src/lib/components/SequenceViewer.react.js

VeraZab · 2019-01-18T17:08:24Z

Thanks for fixing a lot of these up Pierre.
However, I think that if you have a bit of time, what we should do is to set up automatic code formatting for this repository.

For js a popular way to do this is to set up prettier: https://github.com/prettier/prettier
Prettier can be configured so that each save, automatically formats the code for you in your editor. It can also be ran on the entirety of a codebase to output files formatted according to an agreed upon set of rules. Looks like dash-core-components uses it and the rules they're using are here: https://github.com/plotly/dash-core-components/blob/master/.prettierrc

For python, I'd have to double check if there is an agreed upon convention at Plotly. I've posted a question on that in #dash-private.

In another pr, would you like to give a shot at setting up prettier for this repository? You can take inspiration from the dash-core-components repo as it uses it, and go through the docs of prettier to set it up.

VeraZab · 2019-01-18T18:06:08Z

So for Python, looks like there is no consensus yet on the dash-team, so flake8 and manual fixing still seems to be the way unfortunately. We could still add prettier though, for js formatting.

Bachibouzouk · 2019-01-22T17:53:15Z

@VeraZab @shammamah @mkcor : should we merge this PR (once the last two errors from ESlint are solved, cf #114 ) and open a new one to tackle the next failing checks as now pylinter and flake8 are happy ?

mkcor · 2019-01-22T18:02:16Z

Yes! Please rename the PR accordingly (something along the lines of "Fix flake8 and pylint errors") and then feel free to ask for review/approval. Thanks for all the clean-up work!

Bachibouzouk · 2019-01-22T18:42:52Z

@shammamah @VeraZab @mkcor : the problem was the tests/requirements.txt file which was missing packages, now the tests pass on the python side :) \o/

mkcor · 2019-01-22T21:18:16Z

It's amazing to see the tests pass for Python 3.6!! We will probably want to include other versions of Python 3. Locally, I'm developing in Python 3.7...

Bachibouzouk · 2019-01-23T00:03:29Z

close #109 #111

Bachibouzouk added 2 commits January 18, 2019 11:10

Fix eslint errors in Circos component

2181ac6

Fix eslint errors in Ideogram component

cdc96f0

mtwichan temporarily deployed to dash-bio-test-pr-110 January 18, 2019 10:54 Inactive

Fix eslint errors in Molecule3DViewer component

840d441

mtwichan temporarily deployed to dash-bio-test-pr-110 January 18, 2019 11:04 Inactive

mtwichan temporarily deployed to dash-bio-test-pr-110 January 18, 2019 11:30 Inactive

Fix eslint errors in Speck component

c9a8cbd

Bachibouzouk force-pushed the fix_eslint_errors branch from 29f4c07 to c9a8cbd Compare January 18, 2019 11:40

mtwichan temporarily deployed to dash-bio-test-pr-110 January 18, 2019 11:40 Inactive

Fix eslint errors in SequenceViewer component

fb52125

mtwichan temporarily deployed to dash-bio-test-pr-110 January 18, 2019 11:55 Inactive

Bachibouzouk commented Jan 18, 2019

View reviewed changes

Regenerate tarball

bbafce1

mtwichan temporarily deployed to dash-bio-test-pr-110 January 18, 2019 12:08 Inactive

Bachibouzouk requested review from shammamah-zz and VeraZab January 18, 2019 12:09

Bachibouzouk changed the title ~~Fix eslint errors in .react.js files~~ Fix CircleCI failed checks Jan 18, 2019

This was referenced Jan 18, 2019

CircleCI checks are failing due to eslint errors in certain components' .react.js files #109

Closed

CircleCI checks are failing due to pylint and flake8 errors in most demo apps .py files #111

Closed

Bachibouzouk added 4 commits January 18, 2019 14:27

Fix pylint and flake8 errors in app_manhattan_plot.py

cab52b0

Fix pylint and flake8 errors in app_volcano_plot.py

08ef7ad

Fix pylint and flake8 errors in app_needle_plot.py, except PEP8 E501

1ce43f1

Fix pylint and flake8 errors in app_nemolecule3d.py

a448abe

mtwichan temporarily deployed to dash-bio-test-pr-110 January 18, 2019 13:55 Inactive

Fix typo in app_volcano_plot.py

b862f1f

mtwichan temporarily deployed to dash-bio-test-pr-110 January 18, 2019 14:00 Inactive

shammamah-zz mentioned this pull request Jan 21, 2019

Set up Prettier for JS components. #112

Closed

Merge branch 'master' into fix_eslint_errors

9f551e5

Fix misc pylint errors which appear only online

4dbb512

mtwichan temporarily deployed to dash-bio-test-pr-110 January 22, 2019 15:45 Inactive

Fix pylint errors in app_clustergram.py

e6c2f7a

mtwichan temporarily deployed to dash-bio-test-pr-110 January 22, 2019 16:17 Inactive

Bachibouzouk added 4 commits January 22, 2019 17:21

Add biopython to requirements.txt

4893beb

Ignore pylint warnings for callbacks

aaad7ef

Fix pylint command

f6bd62a

Fix flake8 command

ff372c4

mtwichan temporarily deployed to dash-bio-test-pr-110 January 22, 2019 16:39 Inactive

Save up line space while keeping pylint rating

37500ae

mtwichan temporarily deployed to dash-bio-test-pr-110 January 22, 2019 17:01 Inactive

Bachibouzouk added 4 commits January 22, 2019 18:38

Fix indentation errors in app_circos.py

78e063f

Fix E714 in app_clustergram.py

ffb6771

Fix indent in test_volcano_plot.py

33644d8

Ignore pylint 'too-many-lines' warning

ea59468

mtwichan temporarily deployed to dash-bio-test-pr-110 January 22, 2019 17:42 Inactive

Bachibouzouk moved this from In progress to Pending review in Sprint 3 Jan 22, 2019

Bachibouzouk changed the title ~~Fix CircleCI failed checks~~ Fix CircleCI pylint, esline and flake8 failed checks Jan 22, 2019

Fix tests/requirements.txt

e4b7f19

mtwichan deployed to dash-bio-test-pr-110 January 22, 2019 18:38 View deployment

mkcor approved these changes Jan 22, 2019

View reviewed changes

Sprint 3 automation moved this from Pending review to Needs merge Jan 22, 2019

Bachibouzouk merged commit 7616aa3 into master Jan 22, 2019

Sprint 3 automation moved this from Needs merge to Done Jan 22, 2019

VeraZab deleted the fix_eslint_errors branch January 23, 2019 15:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix CircleCI pylint, esline and flake8 failed checks #110

Fix CircleCI pylint, esline and flake8 failed checks #110

Bachibouzouk commented Jan 18, 2019 •

edited

Loading

Bachibouzouk left a comment

Bachibouzouk Jan 18, 2019

Bachibouzouk Jan 18, 2019

Bachibouzouk Jan 18, 2019

Bachibouzouk Jan 18, 2019

shammamah-zz Jan 18, 2019

shammamah-zz Jan 18, 2019

Bachibouzouk Jan 21, 2019 •

edited

Loading

Bachibouzouk Jan 18, 2019

shammamah-zz Jan 18, 2019

Bachibouzouk Jan 21, 2019

VeraZab commented Jan 18, 2019

VeraZab commented Jan 18, 2019

Bachibouzouk commented Jan 22, 2019

mkcor commented Jan 22, 2019

Bachibouzouk commented Jan 22, 2019 •

edited

Loading

mkcor commented Jan 22, 2019

Bachibouzouk commented Jan 23, 2019 •

edited

Loading

		@@ -26,7 +26,7 @@ export default class Circos extends Component {
		* Used to set a click or hover event on tracks/layout that will show annotations for the circos grpah.

		@@ -192,13 +192,14 @@ export default class Ideogram extends Component {

		if (this.props.setProps) {
		this.tooltipData = document.getElementById('_ideogramTooltip').innerHTML;

		@@ -115,6 +114,7 @@ export default class Speck extends Component {
		}

		// finally apply the user-supplied view parameters

		// add event listeners
		const interactionHandler = new speckInteractions(this, renderer, container);

Fix CircleCI pylint, esline and flake8 failed checks #110

Fix CircleCI pylint, esline and flake8 failed checks #110

Conversation

Bachibouzouk commented Jan 18, 2019 • edited Loading

Dash Bio pull request

Before asking for a review

PR merging checklist

DDS Deployment

DDS Deployment Debugging

Windows

OS X

Bachibouzouk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bachibouzouk Jan 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VeraZab commented Jan 18, 2019

VeraZab commented Jan 18, 2019

Bachibouzouk commented Jan 22, 2019

mkcor commented Jan 22, 2019

Bachibouzouk commented Jan 22, 2019 • edited Loading

mkcor commented Jan 22, 2019

Bachibouzouk commented Jan 23, 2019 • edited Loading

Bachibouzouk commented Jan 18, 2019 •

edited

Loading

Bachibouzouk Jan 21, 2019 •

edited

Loading

Bachibouzouk commented Jan 22, 2019 •

edited

Loading

Bachibouzouk commented Jan 23, 2019 •

edited

Loading