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

denote-export: linting and test failures #155

Merged
merged 4 commits into from
Jul 22, 2024

Conversation

jayrajput
Copy link
Contributor

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
Copy link
Contributor Author

How can I re-trigger the CI for linting?

@jayrajput
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@jayrajput
Copy link
Contributor Author

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
Copy link
Owner

ahyatt commented Jul 21, 2024

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
Copy link
Contributor Author

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
Copy link
Owner

ahyatt commented Jul 21, 2024

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
Copy link
Contributor Author

jayrajput commented Jul 22, 2024

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 (??).

@jayrajput jayrajput changed the title documentation string updates for linting errors. ekg-denote-export: linting and test failures Jul 22, 2024
@jayrajput jayrajput changed the title ekg-denote-export: linting and test failures denote-export: linting and test failures Jul 22, 2024
@ahyatt
Copy link
Owner

ahyatt commented Jul 22, 2024

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.

@ahyatt ahyatt merged commit 8b064fe into ahyatt:denote-export Jul 22, 2024
2 checks passed
ahyatt added a commit that referenced this pull request Jul 28, 2024
* ekg-denote export changes (#144)

Adds export from ekg to denote.

Authored-by: Jay Rajput

* fix #146 (#148)

Co-authored-by: Jay Rajput

* Note new denote export in docs

* Add denote as an extra dependency

* denote-export: linting and test failures (#155)

* documentation string updates for linting errors.

* documentation string updates for linting errors

* update function names to start with ekg-denote-test

* updates for denote changes

---------

Co-authored-by: Jay Rajput <[email protected]>

* Allow emoji tags (#156)

This should fix #149.

* Set version number to 0.6.1 (#157)

* Bump version to 0.7.0

* Rethink version number, let's call the new release 0.6.1

It isn't quite a new release, even though it has some minor functionality / UI
changes (how tag actions happen).

* Enable the CI for the develop branch (#158)

---------

Co-authored-by: Jay Rajput <[email protected]>
Co-authored-by: Jay Rajput <[email protected]>
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