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

[Helpers\helper] Refactor helper.nim #1273

Merged
merged 52 commits into from
Nov 23, 2023
Merged

[Helpers\helper] Refactor helper.nim #1273

merged 52 commits into from
Nov 23, 2023

Conversation

RickBarretto
Copy link
Collaborator

@RickBarretto RickBarretto commented Sep 16, 2023 •
edited
Loading

Description

This PR cleans helper.nim up and refactor some functions, making them more readable and maintainable.

Type of change

  • Code cleanup
  • Unit tests (added or updated unit-tests)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (implementation update, or general performance enhancements)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation (documentation-related additions)

@RickBarretto
apply return early pattern to getInfo
a3d73f9 
* avoids unnecessarily nesting code
@RickBarretto
inlining control-flow
1cc0221
@RickBarretto
group DOCGEN's available attributes
21a992d
@RickBarretto
apply return early pattern to getInfo when v is a function
e48fc13 
* avoids unnecessarily nesting code
@RickBarretto
remove code repetition with validSpec template
c772fdb
@RickBarretto
rename local variables and declare it at the top
5fae0b7 
* improves the readability and avoids ambiguity
@RickBarretto
using collect for a better readability
9488444 
* newBlock isn't copying elements one by one, but doing a shallow copy, so no big problems here
@RickBarretto
organize and format getInfo
71c20c8
@RickBarretto
simplfy code with listType template
b0c5a25 
* avoids code repetition and unnecessary nesting
@github-actions github-actions bot added the helpers Issues related to Helper modules label Sep 16, 2023
@RickBarretto RickBarretto changed the title [helpers] Refactor helper.nim [Helpers\helper] Refactor helper.nim Sep 16, 2023
@RickBarretto RickBarretto marked this pull request as draft September 16, 2023 01:12
@RickBarretto
Copy link
Collaborator Author

RickBarretto commented Sep 16, 2023 •
edited
Loading

@drkameleon I have two questions to do.

  1. What do you think about this?
  2. Are "examples" strictly necessary to be generated when it's a function? (group DOCGEN's available attributes) Or will our docgen just ignore it? I'm afraid about break the documentation generation, but I can decouple it again if you tell me...

@RickBarretto
Copy link
Collaborator Author

RickBarretto commented Sep 16, 2023 •
edited
Loading

@drkameleon, the error it's emitting is this:

image

IDK if this is related to any modification on helper.nim.

@drkameleon
Copy link
Collaborator

@drkameleon I have two questions to do.

  1. What do you think about this?
  2. Are "examples" strictly necessary to be generated when it's a function? (group DOCGEN's available attributes) Or will our docgen just ignore it? I'm afraid about break the documentation generation, but I can decouple it again if you tell me...
  1. Great idea to be honest (I'll have to look at the final rewrite, but I I like your idea!)
  2. Examples are generated only for docgen (basically, when I want to generate the documentation, I first build Arturo accordingly - with ./build.nims docs - and then use that binary for the doc generation). At some point, the examples formed part of all normal binaries too; but honestly having all this text data just packed into every binary, in case sb needs it... I don't know. Regarding "breaking" it... well, go ahead, and we'll see later! haha 😉

@drkameleon
Copy link
Collaborator

@drkameleon, the error it's emitting is this:

image

IDK if this is related to any modification on helper.nim.

Hmm... it doesn't seem like it's related at all - to me, it looks like one of the various leftover bugs with the Quantity implementation (unfortunately). I'll have to go through them one by one, I guess it could be my very next PR ;)

@RickBarretto
use return just for control-flow
01b5003 
this pattern is the best nim way to have a default return and other ones
@RickBarretto
group ident declarations
13cc945
@RickBarretto
replace ugly manual while by range
724f26c
@RickBarretto
create and implement printHeader
293c770
@RickBarretto
create and implement printAlias
7e7a712
@RickBarretto
create and implement printDescription
055d2e1
@RickBarretto
create and implement printFunction
1eff1c5
@RickBarretto
create internal type to use in functions
ed9f6cf 
reason: a lot of functions using objName and objValue
@RickBarretto
use ValueObj internally on printInfo
a863434
@RickBarretto
convert all printInfo helpers to use ValueObj as main parameter
05a77e9
@RickBarretto
use ValueObj internally on getInfo
a281c14
@RickBarretto
convert all getInfo helpers to use ValueObj as parameter
dad222b
@RickBarretto
rename all objInfo to info
0178418
@RickBarretto
minor fix
1965239
@RickBarretto
convert getUsageForFunction to use ValueObj as main parameter
1166d7a
@RickBarretto
minor fix
c13c36b
@RickBarretto
format and refactor printLine
5df4e18 
* replace ugly manual while by range
@RickBarretto
minor improvement
5cacf71
@RickBarretto
replade all fmt("") by fmt""
1a8835e
@RickBarretto
minor fixes
394b1c4
@RickBarretto
[tests\quantities] disable broken test
998b5d1
@github-actions github-actions bot added the unit-test Unit tests label Sep 16, 2023
@RickBarretto RickBarretto marked this pull request as ready for review September 16, 2023 18:29
@RickBarretto
Copy link
Collaborator Author

@drkameleon finished!

If you think I should do more things here, you can tell me!

@drkameleon
Copy link
Collaborator

P.S. I haven't forgotten about this. I just want to take my time and look into properly before merging (and given the amount of work that I've had lately, well... I guess I decided to wait a tiny bit so that I don't do it in a hurry). But I will ;)

@drkameleon drkameleon self-requested a review November 23, 2023 07:14
drkameleon
drkameleon approved these changes Nov 23, 2023
View reviewed changes
Copy link
Collaborator

@drkameleon drkameleon left a comment

Choose a reason for hiding this comment

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

After... almost 2 months, I finally got around to have a look into it lol.

Well, the truth is it's totally looking good (if I spot anything I don't particularly like, I'll look into it at a different moment too, but I really doubt it).

Very organized and clean! 😉

@drkameleon
Copy link
Collaborator

Ready to merge! 🚀

@drkameleon drkameleon merged commit 6118c58 into arturo-lang:master Nov 23, 2023
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@drkameleon drkameleon drkameleon approved these changes

Assignees
No one assigned
Labels
helpers Issues related to Helper modules unit-test Unit tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@RickBarretto @drkameleon