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

script load tests #53

Merged
merged 2 commits into from
Oct 27, 2023
Merged

script load tests #53

merged 2 commits into from
Oct 27, 2023

Conversation

BMaxV
Copy link
Contributor

@BMaxV BMaxV commented Jul 5, 2023

tests to ensure that the loading works when it should and throws errors when it should too.

relative paths on windows confuse me, so we'll have to find some satisfactory solution for that later.

Also, this is only doing 4 out of 6 cases correctly,

I don't know how to create a callable without signature and the "is supposed to be installed" but triggers the "ModuleNotFoundError" one confuse me.

@KennedyRichard
Copy link
Collaborator

Mr. Max,

First of all, thank you for your time and effort to help improve Nodezator, it is much appreciated.

This message is just to acknowledge my awareness of your PR.

The estimated time for me to review it is a couple of weeks, because of the reasons I listed on discord. Although, depending on the complexity of those tasks (specially the release of the new Nodezator version), the actual time may be extended a bit.

I also answered your questions on discord regarding the missing things mentioned here. You can address them if you want, but it is okay if you don't, since they may be a bit more complex. As I said on discord, when it is time for me to review your PR, depending on what's missing I may accept it right away and leave what's missing for the future.

Again, thank you for your PR.

Peace

@KennedyRichard
Copy link
Collaborator

Hello, Mr. BMaxV!

As I said, I couldn't give you an accurate estimate because of all the stuff I had to do. Now 8 weeks after my last message I finally released the new version. As I pointed out in the link shared in my previous post, now I'll proceed to create and publish a new discussion on graph execution algorithms and then I'll finally be able to review your PR.

Just wanted to let you know that your PR is on my radar and I'll eventually get to it. Thank you for your patience.

Peace

@BMaxV
Copy link
Contributor Author

BMaxV commented Sep 6, 2023

Hey, that's cool! Thanks for letting me know! Rock on!

@KennedyRichard
Copy link
Collaborator

Hello again, Mr. @BMaxV!

This week I finally managed to finish all my pending tasks and could finally look into your PR.

I think you did a great job in picking a suitable subsystem to write a test for. As a result, despite the few missing items you mentioned, your test is clear, effective and focus on the right things.

Whenever people contribute to Nodezator or any other project I maintain, like you did here, I think it is only fair to consult them first before adding changes to their work or give them the opportunity to make the changes themselves (when they are interested in doing so). In your case, there's not much that needs to be done. The few tweaks needed are:

  • moving some folders around (just to keep things a bit more organized)
  • adding a few extra lines for the missing tests and
  • some small changes related to repository management/configuration

Because of that, if it is okay with you, I'd like to make those few changes myself. In other words, I'd like to make some changes to your PR and merge it right away. So, please, let me know if this is okay and I'll handle this work right away.

Again, thank you for your contribution. It represents a bit more of stability that you are bringing to Nodezator, by testing an important subsystem. Sorry for taking so long to finally get to it. The Indie Python project consumes a lot of time and requires me to prioritize tasks and progress carefully in order to keep the work effective, healthy and sustainable.

Peace

@BMaxV
Copy link
Contributor Author

BMaxV commented Oct 26, 2023

Yes, of course, feel free to make any changes you want.

@KennedyRichard
Copy link
Collaborator

Made some preliminary changes. I'll merge now and do the remaining work after this merging. This way it is easier to merge without having to deal with conflicting changes.

With this, I conclude my work on this PR (the work will be finished in a new branch created temporarily just for this purpose).

@KennedyRichard KennedyRichard merged commit 999e834 into IndiePython:main Oct 27, 2023
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