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

Targets need to be path namespaced to avoid conflicts #354

Closed
jmthomas opened this Issue Nov 11, 2016 · 3 comments

Comments

Projects
None yet
4 participants
@jmthomas
Member

jmthomas commented Nov 11, 2016

Right now we add every target's lib directory to the Ruby path. This introduces issues if multiple targets try to require the same file name because the first target will "win" and only its file will be used.

The solution would be to require code outside the target to specify the target name in the require path: require 'INST/lib/my_conversion'. However, this is a major breaking change that probably needs COSMOS 4.0.0.

Whatever the solution, we should ensure the REQUIRE keyword in target.txt still works locally. Other code inside a target lib should perhaps use require_relative to ensure it can be moved. Solution should consider how renaming the target directory could potentially break things.

@jasonatball

This comment has been minimized.

Show comment
Hide comment
@jasonatball

jasonatball Jun 13, 2017

Collaborator

Implementation just caused too many potential issues so closing without fixing. Users should be careful not to overload class names when creating custom interfaces, conversions, etc.

Collaborator

jasonatball commented Jun 13, 2017

Implementation just caused too many potential issues so closing without fixing. Users should be careful not to overload class names when creating custom interfaces, conversions, etc.

@ryanatball ryanatball modified the milestone: v4.0.0 Aug 4, 2017

@ZackM-Work

This comment has been minimized.

Show comment
Hide comment
@ZackM-Work

ZackM-Work Sep 1, 2017

Is this dead? Or is it part of v4.0.0. I am unclear.

Just recently came across this issue when trying standardize some things.

Is this dead? Or is it part of v4.0.0. I am unclear.

Just recently came across this issue when trying standardize some things.

@ryanatball

This comment has been minimized.

Show comment
Hide comment
@ryanatball

ryanatball Sep 1, 2017

Member

Its in 4.0.0 though not documented well, or with good examples.

The primary change was to top_level.rb#require_class. It now allows you to pass a ClassName instead of filename in the cosmos config files for things, such as interfaces, that in the past just accepted a filename for the interface class. This allows the class name to include namespacing. So instead of tcpip_client_interface.rb you could pass MyTarget::TcpipClientInterface.

Also, inside of target.txt you should use the REQUIRE keyword to require everything needed by the target. This REQUIRE will always try to load the file from the targets lib folder first, before searching the entire system path. This guarantees that if a target defines a file, it will be required from that target even if a same named file is in a different targets lib folder.

Sorry this isn't more clear, but it is implemented, and there is a solution now.

Member

ryanatball commented Sep 1, 2017

Its in 4.0.0 though not documented well, or with good examples.

The primary change was to top_level.rb#require_class. It now allows you to pass a ClassName instead of filename in the cosmos config files for things, such as interfaces, that in the past just accepted a filename for the interface class. This allows the class name to include namespacing. So instead of tcpip_client_interface.rb you could pass MyTarget::TcpipClientInterface.

Also, inside of target.txt you should use the REQUIRE keyword to require everything needed by the target. This REQUIRE will always try to load the file from the targets lib folder first, before searching the entire system path. This guarantees that if a target defines a file, it will be required from that target even if a same named file is in a different targets lib folder.

Sorry this isn't more clear, but it is implemented, and there is a solution now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment