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.

Sign up for GitHub

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

Jump to bottom

refactor: split tables into individual files #550

Merged
merged 19 commits into from
Jan 26, 2025
Merged

refactor: split tables into individual files #550

merged 19 commits into from
Jan 26, 2025

Conversation

gegoune
Copy link
Collaborator

@gegoune gegoune commented Jan 25, 2025

No description provided.

@alex-courtis alex-courtis mentioned this pull request Jan 25, 2025
Merged
2 tasks
alex-courtis
alex-courtis requested changes Jan 26, 2025
View reviewed changes
Copy link
Member

@alex-courtis alex-courtis left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Works beautifully, removes more code.
Please:

  • Update CONTRIBUTING.md
  • change the operation name from colors to generate

scripts/generate_colors.lua Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
scripts/generate_colors.lua Show resolved Hide resolved
alex-courtis
alex-courtis reviewed Jan 26, 2025
View reviewed changes
Makefile Show resolved Hide resolved
@alex-courtis
Copy link
Member

Whoops, I see you're still changing. I'll wait until you're done :)

@gegoune
Copy link
Collaborator Author

gegoune commented Jan 26, 2025

Sorry for pushing so vigorously to ready for review PR. To stop myself from squeezing even more into this poor PR I would like to ask yo to review once again.

On top of what you already commented on:

  • do we want to break off filetypes table to separate file? My reasoning was that it is one of those 'PR entry points' for new icons, which contributors could benefit from having easier access to.
  • types: I only have very superficial knowledge on the matter

Will update contributing document next.

@alex-courtis
Copy link
Member

  • do we want to break off filetypes table to separate file? My reasoning was that it is one of those 'PR entry points' for new icons, which contributors could benefit from having easier access to.

Absolutely! It's a great source of confusion, and data doesn't belong next to code.

@alex-courtis
Copy link
Member

  • types: I only have very superficial knowledge on the matter

Looks good to me; luals will complain if they're not right.

There are two warnings right now for deprecated... I'll deal with them.

gegoune
gegoune commented Jan 26, 2025
View reviewed changes
CONTRIBUTING.md
@@ -24,8 +19,43 @@ or via your OS package manager e.g. *Arch Linux*:
pacman -S stylua luacheck
```

## Adding icons
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note: I didn't just move the block here, so please review as new.

Copy link
Collaborator

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

Choose a reason for hiding this comment

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

Looks good, reverted contributing changes on #551

.gitattributes Outdated Show resolved Hide resolved
hasecilu
hasecilu approved these changes Jan 26, 2025
View reviewed changes
Copy link
Collaborator

@hasecilu hasecilu left a comment

Choose a reason for hiding this comment

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

Working flawlessly!

alex-courtis
alex-courtis reviewed Jan 26, 2025
View reviewed changes
lua/nvim-web-devicons.lua
@@ -1,9 +1,20 @@
local M = {}

---@alias iconName string Name of the icon

---@class Icon
Copy link
Member

@alex-courtis alex-courtis Jan 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This works. It will warn when you do:

---@param icon_data number
local function get_highlight_foreground(icon_data)

@alex-courtis
Copy link
Member

High level testing:

  • filetypes happy path - exit 0
  • filetypes missing - message, exit 2
  • filetypes bad - exit 2
  • filetypes bad and missing - message, exit 2
  • generate happy path - exit 0
  • generate with bad default file - works but exit 2
  • generate with bad light file - no errors and exit 0
  • generate with missing light file - no errors and exit 0
  • generate with no light files - fails with exit 2, but message is meaningful
  • align happy path - works
  • align bad default - works, exit 2
  • runs as normal

@gegoune
Copy link
Collaborator Author

gegoune commented Jan 26, 2025 •
edited
Loading

We can harden it. Which cases where scripts worked for invalid inputs are worth fixing in your opinion?

generate with no light files - fails with exit 2, but message is meaningful

Seen that. Adding force to rm fixes the issue, will push now.

generate with bad default file - works but exit 2

This is probably most worrisome. I would like to enforce single line icon tables somehow, ideally with external formatter rather than coming up with potentially fragile solution ourselves. Any ideas? You did mention emmylua in #543 (comment) but I am not sure if align_continuous_similar_call_args what we are after, but didn't check beyond having a quick glance.

alex-courtis added a commit that referenced this pull request Jan 26, 2025
@alex-courtis
revert CONTRIBUTING as changes present in #550
f55b09b
alex-courtis
alex-courtis approved these changes Jan 26, 2025
View reviewed changes
Copy link
Member

@alex-courtis alex-courtis left a comment

Choose a reason for hiding this comment

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

This is a huge win and will save a lot of review work.

Makefile
Copy link
Member

Choose a reason for hiding this comment

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

These are all present in CI and work.

CI could be consolidated at some point but not today.

CONTRIBUTING.md
@@ -24,8 +19,43 @@ or via your OS package manager e.g. *Arch Linux*:
pacman -S stylua luacheck
```

## Adding icons
Copy link
Member

Choose a reason for hiding this comment

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

Looks good, reverted contributing changes on #551

@alex-courtis
Copy link
Member

alex-courtis commented Jan 26, 2025 •
edited
Loading

We can harden it. Which cases where scripts worked for invalid inputs are worth fixing in your opinion?

I reckon it's fine - invalid inputs are obvious and will fail CI.

generate with no light files - fails with exit 2, but message is meaningful

Seen that. Adding force to rm fixes the issue, will push now.

Nice. CI will fail anyway, which is what we're after.

generate with bad default file - works but exit 2

This is probably most worrisome. I would like to enforce single line icon tables somehow, ideally with external formatter rather than coming up with potentially fragile solution ourselves. Any ideas? You did mention emmylua in #543 (comment) but I am not sure if align_continuous_similar_call_args what we are after, but didn't check beyond having a quick glance.

Honestly, garbage in, garbage out.

I don't think it's worth it to add/move to emmylua. It's not going to handle cases like this either - it will just format them the same as mini does.
mini is doing align_continuous_similar_call_args so nothing to do here.

This was referenced Jan 26, 2025
Closed
Closed
@gegoune
Copy link
Collaborator Author

gegoune commented Jan 26, 2025

Thanks for reviewing, both!

@gegoune gegoune merged commit 4b0d255 into master Jan 26, 2025
5 checks passed
@gegoune gegoune deleted the refactor/split branch January 26, 2025 23:03
alex-courtis added a commit that referenced this pull request Jan 26, 2025
@alex-courtis @hasecilu
docs(#457): add issue templates, notes on adding new icons (#551)
5effc6f 
* docs: update contribution / icon adding docs

* add issue templates

* use type instead of label

* fixup! docs: update contribution / icon adding docs

* revert CONTRIBUTING as changes present in #550

* add missing icon notice

* add contributing reference

---------

Co-authored-by: hasecilu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hasecilu hasecilu hasecilu approved these changes

@alex-courtis alex-courtis alex-courtis approved these changes

@Akmadan23 Akmadan23 Awaiting requested review from Akmadan23

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gegoune @alex-courtis @hasecilu