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

chore: add config.nims with nim defaults & chore tasks #21

Merged
merged 1 commit into from
Jun 2, 2023

Conversation

ZoomRmc
Copy link
Contributor

@ZoomRmc ZoomRmc commented Jun 1, 2023

This PR adds a config.nims that provides:

  • A few compiler defaults (some of them based on PR chore: add nim_test.yml #20) .
  • A Nim task test to run all the units in the repo as executables, with an assumption it runs tests. Task runs with nim test.
    This task is a proposed replacement for the shell scripts from PR chore: add nim_test.yml #20.
    Main benefits over Bash: cross-platform, no additional dependencies.
  • A Nim task prettyfy ro run nimpretty on all *.nim files in the repository. Task runs with nim prettyfy
    This task is a proposed replacement for the shell script from PR chore: add check_code_format workflow #19.

List of compiler defaults:

--gc:arc
--checks:on
--assertions:on
--spellSuggest
--styleCheck:error

@ZoomRmc ZoomRmc requested a review from dlesnoff as a code owner June 1, 2023 16:32
@ZoomRmc ZoomRmc mentioned this pull request Jun 1, 2023
@ZoomRmc ZoomRmc changed the title chore: add config.nims with nim defaults & test task chore: add config.nims with nim defaults & chore tasks Jun 1, 2023
@Panquesito7 Panquesito7 added the enhancement New feature or request label Jun 1, 2023
Copy link
Collaborator

@dlesnoff dlesnoff left a comment

Choose a reason for hiding this comment

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

Apart from being cross-platform, I prefer Nimscript configuration file that does not need additional dependency like Nimble or Atlas.

config.nims Outdated Show resolved Hide resolved
config.nims Show resolved Hide resolved
config.nims Outdated Show resolved Hide resolved
config.nims Outdated Show resolved Hide resolved
config.nims Outdated Show resolved Hide resolved
Copy link
Collaborator

@dlesnoff dlesnoff left a comment

Choose a reason for hiding this comment

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

I think that this is fine, but I wonder how I will merge it with Github actions from previous PRs. I need to ponder about it a bit.

@ZoomRmc
Copy link
Contributor Author

ZoomRmc commented Jun 2, 2023

I think that this is fine, but I wonder how I will merge it with Github actions from previous PRs. I need to ponder about it a bit.

Two ways:

  1. Merge as is, fix jobs and remove .sh files in another PR.
  2. Wait for/propose changes to chore: add check_code_format workflow #19, chore: add nim_test.yml #20.

@vil02
Copy link
Member

vil02 commented Jun 2, 2023

@ZoomRmc The benefits are clear - I fully support your idea.

Regarding nim prettyfy - it does not reformat the files at my end.

@ZoomRmc
Copy link
Contributor Author

ZoomRmc commented Jun 2, 2023

@ZoomRmc The benefits are clear - I fully support your idea.

Thanks.

Regarding nim prettyfy - it does not reformat the files at my end.

Works on my end. It's rather silent, though. Did you reset your working tree to main? It should make the changes in the dynamic_programming dir.

@vil02
Copy link
Member

vil02 commented Jun 2, 2023

Regarding nim prettyfy - it does not reformat the files at my end.

Works on my end. It's rather silent, though. Did you reset your working tree to main? It should make the changes in the dynamic_programming dir.

I have introduced some incorrect formatting and it was not fixed. I have nimpretty 0.2 and Nim Compiler Version 1.6.12 [Linux: amd64].

@ZoomRmc
Copy link
Contributor Author

ZoomRmc commented Jun 2, 2023

I have introduced some incorrect formatting and it was not fixed. I have nimpretty 0.2 and Nim Compiler Version 1.6.12 [Linux: amd64].

Well did the nimpretty run or not with nim prettyfy? This task does little besides that. Nimpretty is not super-reliable and definitely needs a bit more love.

@dlesnoff
Copy link
Collaborator

dlesnoff commented Jun 2, 2023

It worked well on my end

@dlesnoff dlesnoff merged commit dff592d into TheAlgorithms:main Jun 2, 2023
@vil02
Copy link
Member

vil02 commented Jun 2, 2023

I have introduced some incorrect formatting and it was not fixed. I have nimpretty 0.2 and Nim Compiler Version 1.6.12 [Linux: amd64].

Well did the nimpretty run or not with nim prettyfy? This task does little besides that. Nimpretty is not super-reliable and definitely needs a bit more love.

Now I could check it again: it turned out that introducing some big number of empty lines is not considered as incorrect format - sorry for the confusion, it works well.

@vil02
Copy link
Member

vil02 commented Jun 2, 2023

@ZoomRmc I am having another issue. When running the command nim test, I get the error (both locally and here):
/home/runner/.choosenim/toolchains/nim-1.6.12/lib/pure/parseutils.nim(426, 21) Error: system module needs: nimDestroyAndDispose

Same happens for nim prettyfy.

When I do not use --gc: arc, everything works well.

@ZoomRmc
Copy link
Contributor Author

ZoomRmc commented Jun 2, 2023

@vil02 good catch, thanks.

That's a compiler error, it's fixed on devel (not yet sure at which point though). For mitigation we can move the gc switch from config.nims to nim.cfg.

@vil02
Copy link
Member

vil02 commented Jun 2, 2023

@ZoomRmc Thanks, cf. 9c9c383 in #20.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants