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

Update setEdgeTerminal to remove unused ports from style #115

Merged
merged 7 commits into from
Dec 10, 2021

Conversation

haensch-mauricio
Copy link

No description provided.

@coveralls
Copy link

coveralls commented Dec 3, 2021

Coverage Status

Coverage remained the same at 89.483% when pulling 8058d2b on fb-ASIM-4512-links-data into b9190da on master.

Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

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

Good work!

Left a few comments, please take a look.

src/qmxgraph/page/api.js Outdated Show resolved Hide resolved
src/qmxgraph/page/utils.js Outdated Show resolved Hide resolved
src/qmxgraph/page/utils.js Show resolved Hide resolved
src/qmxgraph/page/utils.js Outdated Show resolved Hide resolved
Copy link
Member

@prusse-martin prusse-martin left a comment

Choose a reason for hiding this comment

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

I will wait to seen the testes =)

src/qmxgraph/page/api.js Outdated Show resolved Hide resolved
src/qmxgraph/page/utils.js Show resolved Hide resolved
Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

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

LGTM, great work!

src/qmxgraph/page/utils.js Show resolved Hide resolved
tests/test_js_utils.py Show resolved Hide resolved
tests/test_js_utils.py Show resolved Hide resolved
assert (
graph.eval_js_function('graphs.utils.setStyleKey', 'foobar=3', 'bar', 5) == 'foobar=3;bar=5'
)
assert (
Copy link
Member

Choose a reason for hiding this comment

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

Not a problem.
Don't need to change.
A formatting nitpick.

I think that using a partial will prevent precommit hook from breaking those lines.

    set_style_key = partial(graph.eval_js_function, 'graphs.utils.setStyleKey')
    assert set_style_key('foobar=3;fizzbar=7', 'bar', 5) == 'foobar=3;fizzbar=7;bar=5'

Copy link
Member

@nicoddemus nicoddemus Dec 7, 2021

Choose a reason for hiding this comment

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

On that topic, is there a JS autoformatter we can use with pre-commit?

EDIT: https://prettier.io?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

@oliveira-mauricio don't worry about doing it in this PR though, we can add the configuration in a follow up.

Copy link
Author

Choose a reason for hiding this comment

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

Sure, I will create a task to tackle that later.

#117

@prusse-martin
Copy link
Member

Docs generation is broken on master.

@haensch-mauricio haensch-mauricio changed the base branch from master to rb-ASIM-v1.10.0 December 7, 2021 12:49
Copy link
Member

@prusse-martin prusse-martin left a comment

Choose a reason for hiding this comment

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

I think you could fast forward the RB to master, this will avoid alot of noise in this PR.

src/qmxgraph/page/api.js Outdated Show resolved Hide resolved
src/qmxgraph/page/api.js Outdated Show resolved Hide resolved
@haensch-mauricio haensch-mauricio changed the base branch from rb-ASIM-v1.10.0 to master December 10, 2021 18:05
@haensch-mauricio haensch-mauricio changed the base branch from master to rb-ASIM-v1.10.0 December 10, 2021 18:05
@haensch-mauricio haensch-mauricio merged commit a230a20 into rb-ASIM-v1.10.0 Dec 10, 2021
@haensch-mauricio haensch-mauricio deleted the fb-ASIM-4512-links-data branch December 10, 2021 18:14
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

4 participants