Skip to content

Conversation

@brichet
Copy link
Contributor

@brichet brichet commented Dec 9, 2022

This PR adds a new request to copy a variable to the global scope, and tests on the feature.

It's a draft PR until microsoft/debugpy#1147 is released.

@brichet brichet force-pushed the feature/copy_to_globals branch from 8341b94 to 2df0ad3 Compare January 17, 2023 16:29
@brichet brichet marked this pull request as ready for review January 19, 2023 09:12
std::string m_debugpy_port;
nl::json m_debugger_config;
py::object m_pydebugger;
std::string m_debugpy_version;
Copy link
Member

@JohanMabille JohanMabille Jan 19, 2023

Choose a reason for hiding this comment

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

I think storing a boolean stating if the copy_to_globals_request is available might be more efficient (you perform the comparison only once during the initialization of the debugger).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I thought we could keep it generic for future changes.


std::string expression = "debugpy.__version__";
py::object version = eval(py::str(expression));
m_debugpy_version = version.cast<std::string>();
Copy link
Member

@JohanMabille JohanMabille Jan 19, 2023

Choose a reason for hiding this comment

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

A good idea here would be to replace the . characters in m_debug_py with spaces, to ease future comparisons:

auto debugpy_version = version.cast<std::string>();
std::replace(debugpy_version.begin(), debugpy_version.end(), '.' , ' ');

You can then initialize the boolean member by calling a comparison function with debugpy_version and the minimal version of debugpy in the same format (i.e. "1 6 5");

Copy link
Contributor Author

@brichet brichet Jan 19, 2023

Choose a reason for hiding this comment

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

I don't see the difference if we replace '.' with ' '.
Can it simplify the comparison ?

EDIT: related to the use of istream_iterator in next comment ?

Copy link
Member

Choose a reason for hiding this comment

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

Yes because we can then use the istream_iterator as a tokenizer (it skips the spaces in a string by default, see the implementation in the comment below).

// Compare 2 version and return true if version1 < version2.
// The versions must be strings formatted as the regex: [0-9]+(\.[0-9]+)*
bool less_than_version(std::string version1, std::string version2)
{
Copy link
Member

Choose a reason for hiding this comment

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

If we assume that version1 and version2 have the format "MAJOR MINOR PATCH", then the implementation of this function can be improved:

using iterator_type = std::istream_iterator<int>;
std::istringstream iv1(version1), iv2(version2);
return std::lexicographical_compare(
    iterator_type(iv1),
    iterator_type(),
    iterator_type(iv2),
    iterator_type()
);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @JohanMabille.
Not sure we can assume it as users can still have alpha/beta versions I thinks.

Copy link
Member

@JohanMabille JohanMabille Jan 19, 2023

Choose a reason for hiding this comment

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

In that case we need to split the string to remove the alpha / beta part.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, let's do it this way

nl::json m_debugger_config;
py::object m_pydebugger;
std::string m_debugpy_version;
bool m_version_lt_1_6_5;
Copy link
Member

@JohanMabille JohanMabille Jan 19, 2023

Choose a reason for hiding this comment

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

I think that m_copy_to_globals_available would be more explicit, the fact that it is available after version 1.6.5 only is an implementation detail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right, thanks.

m_copy_to_globals_available = less_than_version(version, "1.6.5");

// Format the version to match [0-9]+(\s[0-9]+)*
int pos = version.find_first_of("abrc");
Copy link
Member

Choose a reason for hiding this comment

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

find_first_pos returns an unsigned int, so this should be size_t pos = ... . Then pos must be compared to std::string::npos: if (pos != std::string::npos) ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I first tried it but I was comparing it to -1 and the compiler was not OK 😄

@JohanMabille JohanMabille merged commit 982653f into jupyter-xeus:main Jan 20, 2023
@JohanMabille
Copy link
Member

Thanks!

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.

2 participants