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

Use Dragonfly's new timers and update Sikuli code #577

Merged
merged 6 commits into from
Jun 26, 2019

Conversation

comodoro
Copy link
Contributor

In order to remove the ugly deprecation warning. @Danesprite could you please look at the usage in stackitems.py at the bottom and confirm that this is the intended usage?

Sikuli code was made into a class to avoid globals. By the way "terminate sick server" without having Sikuli commands raises an exception for me on develop, which should be solved here.

Looking at Dragonfly git log, this change was not released yet, so this should wait until then and dependencies will have to be bumped.

Made the whole thing a class in order to avoid globals
@LexiconCode LexiconCode added Caster Issues pertaining to primarily the Caster project. Enhancement Enhancement of an existing feature labels Jun 12, 2019
@drmfinlay
Copy link
Member

@comodoro The usage looks good to me. I should be able to release dragonfly2 version 0.15.0 today or tomorrow. I think the TimerForWSR class can be deleted in this PR too.

This is related to issue #554.

@comodoro
Copy link
Contributor Author

@Danesprite Thank you. I will remove the old timer class, thanks. As for the new release, whenever you want, I don't think this is pressing.

@drmfinlay
Copy link
Member

No problem. I agree this isn't pressing, there are just enough unreleased changes now that there should be a new release version.

@LexiconCode LexiconCode removed the Caster Issues pertaining to primarily the Caster project. label Jun 21, 2019
@LexiconCode
Copy link
Member

is this considered complete?

@drmfinlay
Copy link
Member

@LexiconCode Although I should be releasing dragonfly2 version 0.15.0 soon (took longer than expected), this PR doesn't really require it. @comodoro mentioned an error with the "terminate sick server" command. Perhaps that could also be fixed here?

This PR makes significant changes to the Sikuli code, so I'll edit the name.

@drmfinlay drmfinlay changed the title Use Dragonfly's new timers Use Dragonfly's new timers and update Sikuli code Jun 24, 2019
@comodoro
Copy link
Contributor Author

@Danesprite @LexiconCode Can be merged then, the sikuli problem was just an exception on exiting, I don't think being a problem in itself, and does not happen ( was resolved ) in this branch/PR.I wanted to increase the version in requirements as well, but it can be done anytime after.

@drmfinlay
Copy link
Member

@comodoro Ah, okay then 👍

@LexiconCode
Copy link
Member

Why freeze the requirements '=='?

setup.py Show resolved Hide resolved
@comodoro
Copy link
Contributor Author

This is after reading some discussions, so that setup.py does contain only restrictions on packages that do not work and we know that and requirements.txt contains packages we know do work. I only know there was something in Dragonfly fairly recently that Caster uses. Feel free to adjust this.

@LexiconCode
Copy link
Member

This is after reading some discussions, so that setup.py does contain only restrictions on packages that do not work and we know that and requirements.txt contains packages we know do work. I only know there was something in Dragonfly fairly recently that Caster uses. Feel free to adjust this.

That makes a lot of sense.

@LexiconCode LexiconCode added Caster Issues pertaining to primarily the Caster project. Sikulix Issues related to Sikulix labels Jun 24, 2019
@LexiconCode
Copy link
Member

I'm running into a few issues and trying to tease out whether it's related to this pull request or upstream in Sikulix

@LexiconCode
Copy link
Member

LexiconCode commented Jun 26, 2019

Once conflicts are resolved like to get this merged.

@comodoro
Copy link
Contributor Author

I'm on it. I admit I am not convinced about the mass removal of rdescripts, even where spec is not clear about what the command does. As for star imports, I'm not a fan, but there are positive aspects.

@LexiconCode
Copy link
Member

LexiconCode commented Jun 26, 2019

Thanks for your help with this!

It's good to have feedback on these matters so thank you.

  1. Regarding removal of rdescripts. Overall it simplifies maintaining code. As we go along and if you have any suggestions we will be adding back some to clarify commands that are ambiguous. Let me know if there's anything in particular or feel free to make pull requests.

  2. As for the * or standard imports. It has its advantage in making easier to add new grammars but it does take away readability. Fortunately if this doesn't pan out it's a relatively minor change. Let's run with it for a bit and see how it goes overall.

The main thread for both of these changes is #589 as a way to centralize discussion.

@LexiconCode LexiconCode merged commit 45b2c03 into dictation-toolbox:develop Jun 26, 2019
@mrob95
Copy link
Member

mrob95 commented Jun 26, 2019

Regarding star imports, I realise that they are frowned upon in Python and I understand why. In our case though, where we are basically importing the same things over and over again, and where easy experimentation is desirable, I think it's the right choice.

Another thing to consider is that this makes it a lot easier to change the way objects are loaded. For example when Aenea support was added, dragonfly imports had to be manually changed in ~50 files, whereas now the same change would just need to be done in one place.

@mrob95 mrob95 mentioned this pull request Jun 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Caster Issues pertaining to primarily the Caster project. Enhancement Enhancement of an existing feature Sikulix Issues related to Sikulix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants