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 org src block feature #411

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

Pierre-Andre
Copy link

This PR aims at providing a dumb-jump "definition search" in org-mode src-block. It closes (in some way) issue #135.

@jacktasia jacktasia requested a review from phikal January 18, 2022 18:55
Copy link
Contributor

@phikal phikal left a comment

Choose a reason for hiding this comment

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

Not much to say about the actual feature (I'm assuming it does what it should), so I'll just leave some style related comments.

dumb-jump.el Outdated
Comment on lines 2047 to 2049
:action '(("Jump to match" . dumb-jump-result-follow))
:candidates (-zip choices results)
:persistent-action 'dumb-jump-helm-persist-action)
Copy link
Contributor

Choose a reason for hiding this comment

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

These seem to only be whitespace changes?

dumb-jump.el Outdated
Comment on lines 2116 to 2120
(let ((alreadyuptodate
(--filter (string= newlang (plist-get it :language))
proplist)))
(if alreadyuptodate
nil
Copy link
Contributor

Choose a reason for hiding this comment

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

You can drop the let here and use an unless instead of if.

dumb-jump.el Outdated
Comment on lines 2141 to 2142
(if newfindrule
(set-default 'dumb-jump-find-rules newfindrule))
Copy link
Contributor

Choose a reason for hiding this comment

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

A when is preferable to a single if used for side effects.

;; add (if needed) composite language to dumb-jump-language-file-exts
(if (not dumb-jump-search-type-org-only-org)
(if newfileexts
(set-default 'dumb-jump-language-file-exts newfileexts)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above, just with unless.

dumb-jump.el Outdated
emacs-lisp "elisp" elisp "elisp"
R "r" r "r"))
(result (plist-get lookup (intern lang))))
(if result result nil)))
Copy link
Contributor

Choose a reason for hiding this comment

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

The if is redundant, but I'd also change the entire function to use assoc/alist-get or cond, instead of plist-get.

(alist-get lang '(("sh" . "shell") ("shell" . "shell") ...))

dumb-jump.el Outdated
Comment on lines 2188 to 2189
"Run dumb-jump-fetch-results if searcher installed, buffer is saved,
and there's a symbol under point."
Copy link
Contributor

Choose a reason for hiding this comment

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

The first line of a docstring should be a complete sentence (checkdoc).

Copy link
Contributor

Choose a reason for hiding this comment

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

I actually missed this in a few of the last docstring, that should be fixed too.

dumb-jump.el Outdated
"In org mode, if inside a src block return
associated language or org when outside a src block."
(let ((lang (nth 0 (org-babel-get-src-block-info))))
; if lang exists then create a composite language
Copy link
Contributor

Choose a reason for hiding this comment

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

Use a ;; comment here, the indentation if unnecessary.

@phikal
Copy link
Contributor

phikal commented Jan 29, 2022

I don't think that fixing "unrelated" checkdoc issues should be part of this pull request.

@Pierre-Andre
Copy link
Author

I have undone all "unrelated checkdoc issues" on my local repos. I can push it if needed.

@phikal
Copy link
Contributor

phikal commented Jan 29, 2022 via email

phikal
phikal previously approved these changes Jan 30, 2022
@jacktasia
Copy link
Owner

Looks like the 24.x tests are failing with

Symbol's value as variable is void: org-src--beg-marker

@phikal
Copy link
Contributor

phikal commented Feb 5, 2022

Using (org-)internal variables is bound to trigger issues eventually, but I don't know enough about Org internals to understand what they are being used for. Is there any alternative to using these variables directly? Having dumb-jump depend on a newer version of org would be a sad solution.

@Pierre-Andre
Copy link
Author

Pierre-Andre commented Feb 6, 2022 via email

@jacktasia
Copy link
Owner

The 24.x tests hung. Retriggering them

@Pierre-Andre
Copy link
Author

Well I need to set up a docker image of emacs 24.5 and 24.4 to understand what's going on. It will take much more time…

@Pierre-Andre
Copy link
Author

emacs 24.x tests hung because

  1. silversearch-ag version is too old (2.1.0 needed)
    -> fix: test are done with rg using dumb-jump-force-searcher
  2. org-version is less than 9 and it implies that
    • there is a bug in org-edit-src-exit : when a src-edit buffer is openned with org-edit-src-code and no change is made and the src-buffer is closed with org-edit-src-exit the org buffer is seen (wrongly) as modified -> fix: I added (save-buffer) for these versions
    • there another bug in dumb-jump determination of project-root and the test cannot be done properly -> fix: I set dumb-jump-project

I hope that all will be fixed but as docker tests with 24.5 does not work at all I cannot be sure…

@Pierre-Andre
Copy link
Author

Using the following Dockerfile

FROM debian
## basic downloads clients
RUN apt-get update && \
    apt-get --no-install-recommends install -y \
         ca-certificates \
         curl \
         git \
         openssh-client \
    && rm -rf /var/lib/apt/lists/*

## for emacs building process
RUN apt-get update && \
    apt-get --no-install-recommends install -y \
        autoconf \
        autogen \
        gcc \
        libgnutls28-dev \
        libncurses-dev \
        make \
        pkg-config \
        texinfo \
    && rm -rf /var/lib/apt/lists/*



######## silvergrep
RUN apt-get update && \
    apt-get --no-install-recommends install -y \
        silversearcher-ag  \
    && rm -rf /var/lib/apt/lists/*

######## ripgrep
RUN curl -LO https://github.com/BurntSushi/ripgrep/releases/download/0.10.0/ripgrep_0.10.0_amd64.deb && dpkg -i ripgrep_0.10.0_amd64.deb && rm ripgrep_0.10.0_amd64.deb && rm -f /var/cache/apt/archives/*.deb /var/cache/apt/archives/partial/*.deb /var/cache/apt/*.bin || true

######## Emacs
RUN cd / && mkdir -p /src && cd /src \
  && git clone --depth 1 git://git.sv.gnu.org/emacs.git emacs
 RUN cd /src/emacs   \
     && ./autogen.sh \
     && ./configure  --without-gif\
                          --with-x-toolkit=no \
                          --without-x \
                          --without-all \
                          --with-gnutls \
                          --prefix="/usr/local" \
     && make  && make install
# #
# ######### Cask
ENV PATH="/root/.cask/bin:$PATH"
RUN mkdir -p /CASK && cd /CASK \
     && git clone https://github.com/cask/cask /CASK/.cask
ENV PATH="/CASK/.cask/bin:$PATH"

######### misc
RUN mkdir -p DUMBJUMP
WORKDIR /DUMBJUMP


ENTRYPOINT ["bash"]

I can run the snapshot version and the

  • dumb-jump-goto-file-line-test failed (the (mock (ring-insert * *)) throws an error and does not seem to be useful here)
  • other tests passed

(NB: EVM emacs-git-snapshot recipe does not work on my laptop.)

Conclusion: these check problems on "snapshot" are not coming from org-mode

@NightMachinery
Copy link

Any updates?

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.

4 participants