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

chg: usr: Add support for subnodes #92

Merged
merged 1 commit into from Aug 9, 2022
Merged

Conversation

sergiosmcr
Copy link
Contributor

No description provided.

@sergiosmcr sergiosmcr marked this pull request as draft July 21, 2022 00:48
from .graph import TopologyGraph


__all__ = ["TopologyGraph", "Link", "Node", "Port"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
__all__ = ["TopologyGraph", "Link", "Node", "Port"]
__all__ = ['TopologyGraph', 'Link', 'Node', 'Port']

"""
Raised when there are inconsistencies in the topology.
"""
pass
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
pass
pass
__all__ = [
'NotFound',
'AlreadyExists',
'Inconsistent',
]

from typing_extensions import Self
from warnings import warn

from .exceptions import Inconsistent, NotFound, AlreadyExists
Copy link
Contributor

Choose a reason for hiding this comment

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

Narrow to large or large to narrow, choose one and stick with it. Use narrow to large.

"""
if node.identifier in self._nodes:
raise AlreadyExists(
f"Node {node.identifier} already exists in the topology")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
f"Node {node.identifier} already exists in the topology")
f'Node {node.identifier} already exists in the topology'
)

"""
if link.identifier in self._links:
raise AlreadyExists(
f"Link {link.identifier} already exists in the topology")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
f"Link {link.identifier} already exists in the topology")
f'Link {link.identifier} already exists in the topology'
)

:param port_id: The id of the port to return.
"""
if port_id not in self._ports:
raise NotFound(f"Port {port_id} not found in the topology")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
raise NotFound(f"Port {port_id} not found in the topology")
raise NotFound(f'Port {port_id} not found in the topology')

This method exists for backwards compatibility. Use links() instead.
"""
warn(
"TopologyGraph.bilinks() is deprecated. Use links() instead.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"TopologyGraph.bilinks() is deprecated. Use links() instead.",
'TopologyGraph.bilinks() is deprecated. Use links() instead.',

f"{link.node2.identifier}")


__all__ = ["TopologyGraph"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
__all__ = ["TopologyGraph"]
__all__ = ['TopologyGraph']

return (node_id, port_id) in self.identifier


__all__ = ["Link"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
__all__ = ["Link"]
__all__ = ['Link']

requirements.txt Outdated
@@ -8,3 +8,5 @@ pexpect>=4.6
pyparsing
pynml
pyszn>=1.4.0
objns
typing_extensions
Copy link
Contributor

Choose a reason for hiding this comment

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

Really really needed?

@sergiosmcr sergiosmcr force-pushed the salazar/subnodes branch 2 times, most recently from 1d0f871 to 77b5e15 Compare July 28, 2022 19:29
- Replace NMLExtendedManager by a new TopologyGraph
  class and other classes for Node, Link and Port.
- Drop supports for NML and graphviz plot output
  which implies removing these pytest plugin flags:
  - --topology-nml-dir
  - --topology-plot-dir
  - --topology-plot-format
@sergiosmcr sergiosmcr marked this pull request as ready for review July 28, 2022 19:49
@sergiosmcr sergiosmcr changed the title chg: usr: Add support for subnode @wip chg: usr: Add support for subnodes Jul 28, 2022
mkpath(args.nml_dir)

# Determine NML export directory and create it if required
if args.nml_dir:
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

Love to deprecate old things nobody uses.

Copy link
Contributor

@carlos-jenkins carlos-jenkins left a comment

Choose a reason for hiding this comment

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

Great job. Love the modernization of core concepts of Topology. Great job.

mkpath(args.nml_dir)

# Determine NML export directory and create it if required
if args.nml_dir:
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

Love to deprecate old things nobody uses.

Comment on lines +108 to +112
warn(
"nml attribute is obsolete and will be removed in future "
"versions. Use graph attribute instead.",
DeprecationWarning
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Love it.

@@ -8,3 +8,5 @@ pexpect>=4.6
pyparsing
pynml
pyszn>=1.4.0
typing_extensions
Copy link
Contributor

Choose a reason for hiding this comment

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

I still think this isn't required.

@carlos-jenkins carlos-jenkins merged commit 4f8d893 into master Aug 9, 2022
@carlos-jenkins carlos-jenkins deleted the salazar/subnodes branch August 9, 2022 20:06
@carlos-jenkins
Copy link
Contributor

Great job!

Merging without waiting for CI as Travis blocked our account. Migrating to CircleCI or other in another PR.

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