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

Add out method #285

Merged
merged 19 commits into from
Sep 24, 2021
Merged

Add out method #285

merged 19 commits into from
Sep 24, 2021

Conversation

marimeireles
Copy link
Member

No description provided.

Copy link
Member Author

@marimeireles marimeireles left a comment

Choose a reason for hiding this comment

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

This is a WIP! I'd appreciate your input @JohanMabille :)

src/xinterpreter.cpp Outdated Show resolved Hide resolved
src/xinterpreter.cpp Outdated Show resolved Hide resolved
src/xinterpreter.cpp Outdated Show resolved Hide resolved
@martinRenou
Copy link
Member

@marimeireles could you provide more context concerning this PR?

include/xeus/xinterpreter.hpp Outdated Show resolved Hide resolved
include/xeus/xinterpreter.hpp Outdated Show resolved Hide resolved
src/xinterpreter.cpp Outdated Show resolved Hide resolved
@marimeireles
Copy link
Member Author

I think this is more how you imagined it @JohanMabille?
If yes I'm gonna write some tests for it.

src/xmessage_builder.cpp Outdated Show resolved Hide resolved
src/xmessage_builder.cpp Outdated Show resolved Hide resolved
{
nl::json kernel_res;
kernel_res["status"] = "ok";
kernel_res = data;
Copy link
Member

Choose a reason for hiding this comment

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

Why do you have an extra data argument to this function? Shouldn't this be enough?

nl::json create_successful_reply(const nl::json& payload,
                                 const nl::json& user_expressions)

{
nl::json kernel_res;
kernel_res["status"] = "ok";
kernel_res["code"] = code;
Copy link
Member

Choose a reason for hiding this comment

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

I believe code is not part of the specs for the complete_reply https://jupyter-client.readthedocs.io/en/latest/messaging.html#completion

src/xmessage_builder.cpp Outdated Show resolved Hide resolved
src/xmessage_builder.cpp Outdated Show resolved Hide resolved
test/test_kernel_replies.cpp Outdated Show resolved Hide resolved
src/xmessage_builder.cpp Outdated Show resolved Hide resolved
src/xmessage_builder.cpp Outdated Show resolved Hide resolved
Copy link
Member

@martinRenou martinRenou left a comment

Choose a reason for hiding this comment

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

Looks good!

example/src/custom_interpreter.cpp Outdated Show resolved Hide resolved
test/test_kernel_replies.cpp Outdated Show resolved Hide resolved
include/xeus/xmessage_builder.hpp Outdated Show resolved Hide resolved
{"start", 0}
});

return xeus::create_successful_reply(payload);
Copy link
Member

Choose a reason for hiding this comment

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

You may want to move the payload to avoid costly copy.

test/test_interpreter.cpp Show resolved Hide resolved
@marimeireles
Copy link
Member Author

I haven't finished it yet!
I'm working on some tests. I added other helper functions.

include/xeus/xhelper.hpp Outdated Show resolved Hide resolved
test/test_unit_kernel.cpp Outdated Show resolved Hide resolved
test/test_unit_kernel.cpp Outdated Show resolved Hide resolved
@marimeireles
Copy link
Member Author

so, this is the output I get:

(xeus) mariana@wintermute ~/dev/xeus/build/test (output*?) $ ./test_unit_kernel
[doctest] doctest version is "2.4.6"
[doctest] run with "--help" for options
this print is running 4 ever...

After running the following code:

        TEST_CASE("print_starting_message")
        {
            auto context = make_context<zmq::context_t>();

            using interpreter_ptr = std::unique_ptr<test_kernel::test_interpreter>;
            interpreter_ptr interpreter = interpreter_ptr(new test_kernel::test_interpreter());
            xkernel kernel(get_user_name(),
                         std::move(context),
                         std::move(interpreter),
                         make_xserver_zmq);
            std::string kernel_config = print_starting_message(kernel.get_config());
            bool found = kernel_config.find(" Starting kernel...\n"
                                            "\n"
                                            "If you want to connect to this kernel from an other client, just copy and paste the following content inside of a `kernel.json` file. And then run for example:\n"
                                            "\n"
                                            "# jupyter console --existing kernel.json\n"
                                            "\n"
                                            "kernel.json\n"
                                            "```\n"
                                            "{\n"
                                            "    \"transport\": \"tcp\",\n");
            std::cout << "this print is running 4 ever..." << std::endl;
            REQUIRE_EQ(found, true);
        }

    TEST_CASE("extract_filename")
    {
        std::cout << "the extract_filename is never starting bc of the other test" << std::endl;
        char** argv = new char*;
        argv[0] = (char*)"-f";
        argv[1] = (char*)"connection.json";
        std::string file_name = extract_filename(3, argv);
        REQUIRE_EQ(file_name, "connection.json");
    }

is just stuck in the first test, @JohanMabille

@marimeireles
Copy link
Member Author

It was supposed to print the next thing on the next test: std::cout << "the extract_filename is never starting bc of the other test" << std::endl;

@JohanMabille JohanMabille merged commit f122827 into jupyter-xeus:master Sep 24, 2021
DerThorsten pushed a commit to DerThorsten/xeus that referenced this pull request Sep 27, 2021
Added functions to create msgs
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.

3 participants