Per directory yaml #1759

jorgeecardona · 2020-05-15T14:59:45Z

The present requests add the option of reading a yaml-formatted file in the current working directory loading the title of a tab.

zeusintuivo · 2020-07-10T17:08:41Z

How would the sample .guake.yml file look like? And where could it be located?

jorgeecardona · 2020-07-10T23:54:16Z

@zeusintuivo right now guake.yml could be on any directory, when you land in the directory guake reads the file and extract the title.

I guess it could be extended or modified.

In my own machine, I have now some directories I frequently used with .guake.yml files with only a title, e.g.

title: "backend"

and when I get into that directory the tab has "backed" as title, it's very simple at the moment.

zeusintuivo · 2020-07-22T09:14:56Z

I see, this is an original idea. It could be even better if could perfom custom commands. Like set node version or rbenv or rvm or php version. Perhaps have a command line execution section. And also following that topic a autostart section. Something that would only run once vs command line execution that would run every time you go in the folder?

jorgeecardona · 2020-07-22T09:33:18Z

I am glad you like it ;) I am honestly just interested in the title. I am aware more things are possible as you said, but some seem a bit dangerous to me, or at least it seems it should the responsibility of a different component, like .bashrc or something like that.

Also, the API used for the proof-of-concept seems a bit poor to me honestly, and I would be afraid of adding too many options. Like is ok to set the name of the label twice in less than a second, but you could easily get into problems if you run a large command.

I would leave it just as a way to configure Guake on a per-directory basis.

gsemet · 2021-03-04T14:28:11Z

Closing old PR

jorgeecardona · 2021-03-04T16:49:59Z

Hi, is there a way to reopen this and consider merging this feature?

gsemet · 2021-03-04T17:07:58Z

You can reopen but please update the pull request description and the Reno chunk

Davidy22 · 2021-09-09T00:43:13Z

Going back through issues, found this through an old issue and it looks like it could be nice, reopening and will check later. For now, you may need to rebase your branch to pass test cases.

Davidy22 · 2021-09-10T00:11:19Z

Alright, I've gone and done some quick fixing up with the build errors so now we can look at the content of this. For one, we're going to need some docs written so people can actually know how to format the yaml files

gsemet · 2021-09-16T09:10:52Z

can you confirm if this PR is still ok?

Davidy22 · 2021-09-16T09:20:08Z

Pull request has good stuff but needs a little bit more written before it's a feature to add. We need docs so that people can know that this is a thing that guake will look for, maybe some nice controls in the UI for creating these files and then some testing.

jorgeecardona · 2021-09-23T17:33:08Z

I added a small explanation with an example in the Feature section of the documentation. I imagine we could use this file for more than just the title.

So far I have been using this for over a year and I love it, it makes it really easy to navigate my tabs, though I only get lost when I am in a remote machine via ssh. I haven't thought too much about that case anyway or similar: opening another shell from the original shell.

Davidy22

Alright, tested the branch and it works as expected, so going in a little more in depth now. First order of business, the checks seem to be failing again, so those need to be dealt with. Some structural things as well for potential future development of the file as well.

guake/guake_app.py

Davidy22 · 2021-09-24T04:56:10Z

guake/guake_app.py

    def compute_tab_title(self, vte):
        """Compute the tab title"""
+
+        guake_yml = self.read_current_guake_yaml(vte)
+        if guake_yml is not None:


Maybe we can move these checks into the yaml read function? So if there's more added to the yaml file in the future, there's a bit less boilerplate checking that needs to be rewritten each time.

I moved the is not None internally to the read_current_guake_yaml method. That method is returning a dictionary with the file's content, not just a string with the title, so a minimal check is still required here. I did this thinking that the .guake.yml file might contain more than a title.

jorgeecardona · 2021-09-24T09:03:59Z

Great, I added the encoding. I am not sure how much effor to put on checking in the read_guake_yaml I would like this function to simply return a dataclass with the content of the file and maybe keep it cached somewhere in the parent class, until a change of cwd is detected. That class can be accesed then for some of its content, still some minor checks are required at every step where the content is intended to be used.

Also, I am using the extension .yml but I am unsure if everyone is used to have a `.yaml' instead.

I am happy to hear also if adding typings is in the future plans, fro vte in particular I guess I need to add an import on Vte and the vte argument is a 'Vte.Terminal' right?

Best.

Davidy22 · 2021-09-24T13:40:38Z

The merge from master seems to be failing checks, I'll fix that real quick.

mlouielu · 2021-09-24T14:51:17Z

I would like to have description of this PR update at the OP description.

mlouielu

We should add option to reduce disk load, and audit yaml.safe_load to prevent arbitrary code

mlouielu · 2021-09-24T14:53:45Z

guake/guake_app.py

@@ -1070,9 +1071,37 @@ def recompute_tabs_titles(self):
            page_num = self.get_notebook().page_num(terminal.get_parent())
            self.get_notebook().rename_page(page_num, self.compute_tab_title(terminal), False)

+    def read_current_guake_yaml(self, vte) -> dict:


read_current_directory_guake_yaml()

Or load_current..., more suitable for describing what is has done.

I changed for load_cwd_guake_yaml seems better to me since is shorter.

mlouielu · 2021-09-24T14:56:32Z

guake/guake_app.py

+        try:
+            if guake_yaml.is_file():
+                with guake_yaml.open(encoding="utf-8") as fd:
+                    content = yaml.safe_load(fd)


We should audit yaml.safe_load() as this could be a point to include arbitrary code.

safe_load() should be the safe version of load()

I agree with David. I am not planning in going deep into the yaml implementation to check if safe_load is as safe as they advertise it.

mlouielu · 2021-09-24T14:58:47Z

guake/guake_app.py

    def compute_tab_title(self, vte):
        """Compute the tab title"""
+
+        guake_yml = self.read_current_guake_yaml(vte)


We should add config option (enable, disable) to reduce reading from disk.

guake/guake_app.py

jorgeecardona · 2021-10-02T13:36:23Z

I still have to add a flag in the configuration to activate reading from this file. I will try to finish that during the week.

Regarding caching the content to prevent unnecessary reads, I honestly would first implement a very simple time() based control to prevent accessing the file if it has been accessed less than certain seconds ago. Would that be ok for you guys?

Davidy22 · 2021-10-02T14:04:13Z

Yeah sure that'll probably be fine, generally should use monotonic() instead of time() though for the safety.

Davidy22 · 2021-10-02T14:31:45Z

Back because build failed. For future reference, you can just run make black to handle formatting for you after you're done writing code, I'll make the check pass here for now. Also you can probably squash commits down if you have a couple with the same name

jorgeecardona · 2022-04-18T21:15:20Z

I added an options in prefs and a way to cache the content of the yaml files and prevent reloading too often. I honestly don't like to have the two properties _guake_yml and _guake_yml_load_monotonic but this is a simple solution I guess.

Davidy22 · 2022-04-19T05:08:30Z

CI failing, you might need to rebase your branch onto latest git to resolve the weird dependency thing because I'm not seeing this issue crop up in any of the other active PRs. Looking at the things deepsource is complaining about, you can ignore the second one because it doesn't have anything to do with your changes but this one is relevant.

…ctory

…guake.yml file more than once per second

jorgeecardona · 2022-04-19T09:59:22Z

Hi @Davidy22 I did the rebase to master, but the error is still there. I honestly don't know what's the problem since make test runs for me. Could you suggest a way I can check on my end what's the problem?

jorgeecardona · 2022-04-19T10:18:53Z

The PR is now "ok" according to the last run. The tests are not passing in th CI, but they pass on my machine.

Regarding the code, I was planning to just add a FileManager class in utils.py to handle the cache for file content instead of the two properties in the Guake class. What do you thik about that?

Davidy22 · 2022-04-19T11:07:11Z

Oh, I see you added a pyyaml line to the pipfile, so that probably changes the dependency graph and I suspect that's where the issue is coming from. You can do a pipenv lock to regenerate the lock file from your local setup if you have a working setup and that'll probably clear this up.

Davidy22

Looks like we finally got all checks passing, looks good and works fine, merging.

jorgeecardona mentioned this pull request Jun 3, 2020

Add a per-directory .guake.yml to configure how guake behaves in that directory. #1758

Closed

jorgeecardona force-pushed the per-directory-yaml branch from 4519840 to 7b5eced Compare December 6, 2020 18:48

gsemet closed this Mar 4, 2021

Davidy22 reopened this Sep 9, 2021

Davidy22 requested changes Sep 24, 2021

View reviewed changes

Davidy22 force-pushed the per-directory-yaml branch from 6848181 to a84b557 Compare September 24, 2021 14:25

mlouielu requested changes Sep 24, 2021

View reviewed changes

jorgeecardona force-pushed the per-directory-yaml branch 5 times, most recently from 961853b to aa147f1 Compare April 18, 2022 18:38

jorgeecardona force-pushed the per-directory-yaml branch from aa147f1 to 557bcf2 Compare April 18, 2022 20:03

jorgeecardona added 2 commits April 19, 2022 11:19

Proof of concept for changing the title with a yaml file on each dire…

0923f69

…ctory

Add a boolean to prefs and use time.monotonic to prevent loading the …

d00d4d1

…guake.yml file more than once per second

jorgeecardona force-pushed the per-directory-yaml branch from bf12a5e to d00d4d1 Compare April 19, 2022 09:20

jorgeecardona force-pushed the per-directory-yaml branch 4 times, most recently from 8a9baed to 83ae070 Compare April 19, 2022 13:29

Remove 'anti-pattern'

b7cff88

jorgeecardona force-pushed the per-directory-yaml branch from 83ae070 to 1da807b Compare April 19, 2022 13:41

jorgeecardona added 2 commits April 19, 2022 16:09

Update Pipfile.lock

c0d8975

Use monkeypatch.setenv instead of copying the os.environ for tests.

06f9179

jorgeecardona force-pushed the per-directory-yaml branch from 7a5942e to 06f9179 Compare April 19, 2022 14:09

Davidy22 approved these changes Apr 19, 2022

View reviewed changes

Davidy22 merged commit 985e752 into Guake:master Apr 19, 2022

jorgeecardona deleted the per-directory-yaml branch April 20, 2022 08:44

jorgeecardona mentioned this pull request Apr 20, 2022

Add file loader and tests for compute_tab_table and load_cwd_guake_yml #2076

Merged

Per directory yaml #1759

Per directory yaml #1759

Conversation

jorgeecardona commented May 15, 2020 • edited Loading

zeusintuivo commented Jul 10, 2020

jorgeecardona commented Jul 10, 2020

zeusintuivo commented Jul 22, 2020

jorgeecardona commented Jul 22, 2020

gsemet commented Mar 4, 2021

jorgeecardona commented Mar 4, 2021

gsemet commented Mar 4, 2021

Davidy22 commented Sep 9, 2021 • edited Loading

Davidy22 commented Sep 10, 2021

gsemet commented Sep 16, 2021

Davidy22 commented Sep 16, 2021

jorgeecardona commented Sep 23, 2021

Davidy22 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorgeecardona commented Sep 24, 2021

Davidy22 commented Sep 24, 2021

mlouielu commented Sep 24, 2021

mlouielu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Davidy22 Sep 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorgeecardona commented Oct 2, 2021

Davidy22 commented Oct 2, 2021

Davidy22 commented Oct 2, 2021 • edited Loading

jorgeecardona commented Apr 18, 2022

Davidy22 commented Apr 19, 2022

jorgeecardona commented Apr 19, 2022

jorgeecardona commented Apr 19, 2022

Davidy22 commented Apr 19, 2022

Davidy22 left a comment

Choose a reason for hiding this comment

jorgeecardona commented May 15, 2020 •

edited

Loading

Davidy22 commented Sep 9, 2021 •

edited

Loading

Davidy22 Sep 24, 2021 •

edited

Loading

Davidy22 commented Oct 2, 2021 •

edited

Loading