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

Refactor: use all-the-icons-icon-for-dir #198

Merged
merged 3 commits into from Dec 10, 2019
Merged

Refactor: use all-the-icons-icon-for-dir #198

merged 3 commits into from Dec 10, 2019

Conversation

seagle0128
Copy link
Contributor

No description provided.

Comment on lines 28 to 29
(declare-function all-the-icons-fileicon "ext:all-the-icons.el")
(declare-function all-the-icons-octicon "ext:all-the-icons.el")
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you replace all the logic with all-the-icons-icon-for-dir, are those necessary? They are not being used anywhere now 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, all-the-icons-fileicon and all-the-icons-octicon are used in other functions.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. I'm trying to debug why the linter is complaining 🤔 . So far, if those two lines are removed, it does not complain. But I don't see why it complains if they are being used later in the code 🤔 .

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I've found why it is failing. Those two functions are not really defined, they are generated using the macro define-icon. Code here

That is why the linter is not able to find the function declaration.

@@ -20,10 +20,13 @@

;;; Code:

(require 'cl-lib)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why adding this require ❓

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is for addressing the compiling warnings (not defined: cl-subseq), and cl is deprecated in 27.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was not implying removing this line. I was just curious why (not using Emacs master version, still in Emacs 26.3). I think it would be great to add this change. Sorry I do not explain myself clear 😄

Thank you @seagle0128

@JesusMtnez
Copy link
Contributor

BTW, whenever you can, please update this branch with current master content, since the linter has been updated.

Thank you @seagle0128

@seagle0128
Copy link
Contributor Author

seagle0128 commented Dec 10, 2019

@JesusMtnez Sure. I fixed the linting errors, but the compiling warnings will be there.

Update: It seems the linting process is unable to start in CI.

emacs --batch -q -l .emacs/init.el -l .emacs/elpa/elisp-lint-20180224.2042/elisp-lint.el -f elisp-lint-files-batch dashboard-widgets.el dashboard.el
Loading /root/project/.emacs/.emacs-custom.el (source)...
Loading /root/project/.emacs/dependencies.el (source)...
Cannot open load file: No such file or directory, .emacs/elpa/elisp-lint-20180224.2042/elisp-lint.el
Makefile:19: recipe for target 'lint' failed
make: *** [lint] Error 255

@JesusMtnez
Copy link
Contributor

You need to sync this branch with current master branch to include the CI fix for the linting process.

Update from master branch.
@seagle0128
Copy link
Contributor Author

Okay. Already updated.

@JesusMtnez
Copy link
Contributor

Perfect! Now it is passing!

If you want, restore (require 'cl-lib) to remove that compile warning. If not, I will proceed and merge this PR. 😄

@seagle0128
Copy link
Contributor Author

@JesusMtnez It's fine to merge now. Thanks!

Copy link
Contributor

@JesusMtnez JesusMtnez left a comment

Choose a reason for hiding this comment

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

Thank you for all the work @seagle0128

@JesusMtnez JesusMtnez changed the title Refactor: use all-the-icons-icon-for-dir. Refactor: use all-the-icons-icon-for-dir Dec 10, 2019
@JesusMtnez JesusMtnez merged commit 87779af into emacs-dashboard:master Dec 10, 2019
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