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 API #40

Merged
merged 2 commits into from
Mar 31, 2023
Merged

Add API #40

merged 2 commits into from
Mar 31, 2023

Conversation

kaenguruhs
Copy link
Contributor

This adds an API for external applications, reachable via octopi.local/plugin/TimeToFilament.

@@ -19,6 +21,8 @@ class TimeToFilamentPlugin(octoprint.plugin.SettingsPlugin,
octoprint.plugin.StartupPlugin,
octoprint.plugin.BlueprintPlugin):

variables = dict()
Copy link
Owner

Choose a reason for hiding this comment

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

This should be initialized in __init__ and be called self.variables, otherwise, all instances of this class will share the same data member. Can you please do this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, this is a leftover from a version before :-(

def is_blueprint_csrf_protected(self):
return True

@octoprint.plugin.BlueprintPlugin.route("/", methods=["GET"])
Copy link
Owner

Choose a reason for hiding this comment

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

If we use the / URL then it will make future URLs awkward. Maybe it's better to name this URL. And then if we want future endpoints, those can have different names. You could call it /get_cache like the function or just /get or something else...

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about /api? I would rename the function then, too.

def get_cache(self):
if self._printer.get_state_id() not in (["PRINTING", "PAUSED"]):
return flask.jsonify(None)
addData = self.additional_state_data(False)
Copy link
Owner

Choose a reason for hiding this comment

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

In python we prefer snake_case over camelCase, which is how Java does it. I left some of the JSON keys in camelCase because that's how OctoPrint did it but I would prefer snake case, to be consistent with python's names guidelines.

https://stackoverflow.com/a/8908798/4454

<label class="control-label">Format</label> <div class="controls"><textarea rows=3 class="input-block-level" data-bind="textInput: format"></textarea></div>
<label class="control-label">Enabled</label> <div class="controls"><input type="checkbox" style="margin-left: 20px" data-bind="checked: enabled"></div><br>
<label class="control-label">Format (UI)</label> <div class="controls"><textarea rows=3 class="input-block-level" data-bind="textInput: format"></textarea></div>
<label class="control-label">Format (External)</label> <div class="controls"><textarea rows=3 class="input-block-level" data-bind="textInput: formatExternal"></textarea></div>
Copy link
Owner

Choose a reason for hiding this comment

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

The whitespace looks wrong here. Maybe you used tabs instead of spaces? I prefer spaces for consistency.

if "timeLeft" in dispLines[idx]:
dispLines[idx]["timeLeft"] = max(self._printer.get_current_data()["progress"]["printTimeLeft"]-dispLines[idx]["timeLeft"],0)
import datetime
dispLines[idx]["timeLeftFormatted"] = str(datetime.timedelta(seconds=dispLines[idx]["timeLeft"])).split('.', 2)[0]
Copy link
Owner

Choose a reason for hiding this comment

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

Above I use the "formatDuration" function. Is that javascript, though, or python? Maybe there is a python version, too? Would that look nicer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

formatDuration is JavaScript. I did not find any python-version, and this https://stackoverflow.com/questions/538666/format-timedelta-to-string#comment45123799_538721 pointed me to the used version (just make a string from the timedelta-object, and split off the microseconds.

dispLines[idx]["searchPos"] = self._printer._comm._currentFile.getFilepos()
from jinja2 import Environment, BaseLoader
env = Environment(loader=BaseLoader()).from_string(dispLines[idx]["formatExternal"])
variables = dict_merge(dispLines[idx], __builtins__)
Copy link
Owner

Choose a reason for hiding this comment

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

Based on this usage, it's not clear to me why this is even a class variable... Could it not just be a local variable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should be! I will rename it to jinja_variables and use it as an local variable.

dispLines[idx]["timeLeft"] = max(self._printer.get_current_data()["progress"]["printTimeLeft"]-dispLines[idx]["timeLeft"],0)
import datetime
dispLines[idx]["timeLeftFormatted"] = str(datetime.timedelta(seconds=dispLines[idx]["timeLeft"])).split('.', 2)[0]
dispLines[idx]["dateTimeFinished"] = (datetime.datetime.now()-datetime.timedelta(seconds=dispLines[idx]["timeLeft"])).strftime("%H:%M:%S")
Copy link
Owner

Choose a reason for hiding this comment

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

now - the delta means that this time will be in the past, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, dang. It has to be + instead of - for sure... :'(

@eyal0
Copy link
Owner

eyal0 commented Mar 20, 2023

This adds an API for external applications, reachable via octopi.local/plugin/TimeToFilament.

Thanks for your help!

Can you give an example of an external application that would use this?

Maybe this is a silly question but... why not just simply return the data as-is and let the external app deal with the formatting and stuff? Presumably the external app could do whatever it wants.

@kaenguruhs
Copy link
Contributor Author

Hi eyal0,

you got me - Java and not python is my home turf :-) I will go through your suggested changes and add a commit.
First of all: The application using this API could be Octodash. There was an issue, but it was closed because of the missing API in TimeToFilament: UnchartedBull/OctoDash#1547

To the formatting-thing: Isn't it quite difficult for external applications to decide how to format it right? E.g.: In my setup, it is neccessary to add +1 to the layer (Layer ${parseInt(this.plugins.TimeToFilament["^;LAYER:(\\d+)"].groups[0])+1} in <b>${formatDuration(this.progress.printTimeLeft - this.plugins.TimeToFilament["^;LAYER:(\\d+)"].timeLeft)}</b>) because layering starts with 0. I'd like to have all config for this at one place, not distributed. What do you think?

whitespaces: space instead of tab
format: snake_case instead of camelCase
/api instead of /
octoprint_TimeToFilament/__init__.py Show resolved Hide resolved
@eyal0
Copy link
Owner

eyal0 commented Mar 31, 2023

I'll assume that you tested this alright? This is a pretty low-risk feature to add because it'll only affect people that try to use it.

@eyal0 eyal0 merged commit 2bd6dd6 into eyal0:master Mar 31, 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.

None yet

2 participants