ahyatt / ekg

The emacs knowledge graph, app for notes and structured data.
GNU General Public License v3.0
224 stars 18 forks source link

denote-export: linting and test failures #155

Closed jayrajput closed 1 month ago

jayrajput commented 1 month ago

Updated the documentation string to fix following linting errors (see during code checkin pipeline execution)

[00:00.082]  Linting file ‘ekg-denote.el’
[00:00.094]  ekg-denote.el:38: You should have a section marked ";;; Code:"
[00:00.094]  ekg-denote.el:26: There should be two spaces after a period
[00:00.094]  ekg-denote.el:27: There should be two spaces after a period
[00:00.095]  ekg-denote.el:21: There should be two spaces after a period
[00:00.097]  ekg-denote.el:95: First line is not a complete sentence
[00:00.098]  ekg-denote.el:141: Documentation strings should not start or end with whitespace
[00:00.099]  ekg-denote.el:162: First sentence should end with punctuation
[00:00.101]  ekg-denote.el:192: Error messages should *not* end with a period
[00:00.102]  Found 8 warnings in file ‘ekg-denote.el’
jayrajput commented 1 month ago

How can I re-trigger the CI for linting?

jayrajput commented 1 month ago

Btw, I did checked local linting and that is working fine.

jr@Jays-Mac-mini ekg % eldev lint
eldev lint
Running linter `doc'
File `ekg-auto-save.el': no warnings
File `ekg-denote.el': no warnings
File `ekg-embedding.el': no warnings
File `ekg-llm.el': no warnings
File `ekg-logseq.el': no warnings
File `ekg-org-roam.el': no warnings
File `ekg-test-utils.el': no warnings
File `ekg.el': no warnings

Running linter `package'
[1/1] Installing package `package-lint' (0.23) from `melpa-stable'...
File `ekg-auto-save.el': no warnings
File `ekg-denote.el': no warnings
File `ekg-embedding.el': no warnings
File `ekg-llm.el': no warnings
File `ekg-logseq.el': no warnings
File `ekg-org-roam.el': no warnings
File `ekg-test-utils.el': no warnings
File `ekg.el': no warnings

Running linter `re'
[1/2] Installing package `xr' (1.25) from `gnu'...
[2/2] Installing package `relint' (1.24) from `gnu'...
File `ekg-auto-save.el': no warnings
File `ekg-denote.el': no warnings
File `ekg-embedding.el': no warnings
File `ekg-llm.el': no warnings
File `ekg-logseq.el': no warnings
File `ekg-org-roam.el': no warnings
File `ekg-test-utils.el': no warnings
File `ekg.el': no warnings

Linters have no complaints
jr@Jays-Mac-mini ekg % 
jayrajput commented 1 month ago

I can see linting working fine in the Checks, but now the tests are failing. Looking more into the test failure.

jayrajput commented 1 month ago

Interestingly the test are working fine when I am running them using ert.

Selector: t
Passed:  8
Failed:  0
Skipped: 0
Total:   8/8

Started at:   2024-07-21 21:21:35+0530
Finished.
Finished at:  2024-07-21 21:21:39+0530

........
ahyatt commented 1 month ago

Thanks for these fixes! For retrying, you can click on "Details" and then there should be a button at the top to "Re-Run Jobs". You can also install Eldev locally and run that way, which is a good way to make sure you have fixed everything before checkin.

Would you like me to merge this in now, or would you prefer to get a clean CI check first?

jayrajput commented 1 month ago

I am running eldev locally. Linting passes, but the test is failing. Although ERT is working. I am not able to figure out why eldev tests are failing. Looking for some guidance on how to debug eldev failure.

ahyatt commented 1 month ago

I took a look, and it looks like the problem is that, in denote's current codebase, denote-sluggify takes two arguments, and you only are using one.

jayrajput commented 1 month ago

I fixed both linting and regression tests (CI also showed that is passed). Although I realised that there is a dependency on denote package and whenever denote changes, the code breaks. Is there a way to make things more robust, by pinning down a version of denote. It took me sometime to realise that the ert was working fine as it was using an older version of denote whereas eldev failed as it was rightly pulling the latest denote (??).

ahyatt commented 1 month ago

There is a way to pin the denote version, but only if we have it as an actual dependency of the project. Since we don't want to do that (because it isn't a real dependency, it's optional), we should just document what version of Denote we're expecting at a minimum in the file.