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

Format using nph #54

Merged
merged 11 commits into from
Jun 26, 2024
Merged

Format using nph #54

merged 11 commits into from
Jun 26, 2024

Conversation

bhartnett
Copy link
Contributor

@bhartnett bhartnett commented Jun 26, 2024

This is just a formatting change using nph . and a minor change to add the nph lint check to CI.

@bhartnett bhartnett requested a review from arnetheduck June 26, 2024 08:45
@bhartnett
Copy link
Contributor Author

@arnetheduck Just a heads up that the latest tagged/released version of nph doesn't work with the latest version of nim. The fix is in the master branch but not released yet. This issue should probably be prioritized: arnetheduck/nph#70

@arnetheduck
Copy link
Member

Just a heads up that the latest tagged/released version of nph doesn't work with the latest version of nim.

yeah, I know - as long as it's built on its own via nimble (which is the supported way of building it), it should be fine though.

Comment on lines +24 to +27
VERSION="v0.5.1"
ARCHIVE="nph-linux_x64.tar.gz"
curl -L "https://github.com/arnetheduck/nph/releases/download/${VERSION}/${ARCHIVE}" -o ${ARCHIVE}
tar -xzf ${ARCHIVE}
Copy link
Contributor

@kdeme kdeme Jun 26, 2024

Choose a reason for hiding this comment

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

Perhaps it should instead become a dependency in the nimble file? Did you try if that works?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, yes it works locally for me. I'll try that

@bhartnett
Copy link
Contributor Author

Just a heads up that the latest tagged/released version of nph doesn't work with the latest version of nim.

yeah, I know - as long as it's built on its own via nimble (which is the supported way of building it), it should be fine though.

Yes I see what you mean. Using nim version 2.0.6 when I run nimble install nph, it fails to install the latest tagged version so it falls back to installing the master version which does work.

@bhartnett bhartnett merged commit a691d5b into master Jun 26, 2024
7 of 10 checks passed
@bhartnett bhartnett deleted the format-using-nph branch June 26, 2024 15:00
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.

3 participants