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

Move command line parameter code #20946

Merged
merged 1 commit into from
Nov 30, 2022
Merged

Move command line parameter code #20946

merged 1 commit into from
Nov 30, 2022

Conversation

IgorDeepakM
Copy link
Contributor

Command line paramater code moved from os.nim to cmdparam.nim

@metagn
Copy link
Collaborator

metagn commented Nov 28, 2022

I like the idea but there is no deliberation on this that I know of.

  • should the module name be cmdparam?
  • why is parseCmdLine here? if it's here, shouldn't stuff like quoteShell also be here? I think these would warrant another module (maybe with a typesafe API)

Would be nice to hear your reasoning.

Also new modules tend to go in lib/std, not lib/pure.

@IgorDeepakM
Copy link
Contributor Author

IgorDeepakM commented Nov 28, 2022

I like the idea but there is no deliberation on this that I know of.

* should the module name be `cmdparam`?

* why is `parseCmdLine` here? if it's here, shouldn't stuff like `quoteShell` also be here? I think these would warrant another module (maybe with a typesafe API)

Would be nice to hear your reasoning.

Also new modules tend to go in lib/std, not lib/pure.

When it comes to the module name cmdparam, this was my idea of the name as the module facilitates obtaining och parsing the command line. This can be changed if you have other suggestions.

I put cmdparam.nim in lib/pure because os.nim also is located there. I'm not too familiar with the source code structure and what goes where. cmdparam.nim can be moved to lib/std if that's a better location.

When it comes to the quoteShell function, I didn't include these because these belong more to execution of external processes. parseCmdLine is used more for what commands you receive from the command line.

Just to clarify the background and reason for this change. In order to use command line parameters you need to include the os module and this brings in the whole lot with many functionalities that might not even exist for the particular embedded system. For embedded systems, everything might not be implemented. Instead you can single out the command line functionality from a separate module by "import std/cmdparam". The goal is that you should be able to cherry pick functionality more rather than a big os module monolith.

@Araq
Copy link
Member

Araq commented Nov 28, 2022

The refactoring is great but the module's name should be std / cmdline.

@IgorDeepakM
Copy link
Contributor Author

IgorDeepakM commented Nov 29, 2022

One check was cancelled after 60 minutes. There is not much information why this test failed other than the text.

Test selected Nimble packages
  testament --batch:0_3 pcat nimble-packages
  Downloading Official package list
      Success Package list downloaded.
  Error: The operation was canceled.

Any idea what this is about?

@ringabout
Copy link
Member

Never mind. The test sometimes fails for unknown reasons. I have restarted it.

Comment on lines 3 to 4
# Nim's Runtime Library
# (c) Copyright 2015 Andreas Rumpf
Copy link
Member

Choose a reason for hiding this comment

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

The copyright year can be updated since it is a new module.

Command line paramater code moved from os.nim to cmdparam.nim
@metagn
Copy link
Collaborator

metagn commented Nov 29, 2022

Do we add an extra std/shell for stuff like quoteShell or should they go here?

Also you don't have to bother manually squashing & force pushing every time, it's done when merging anyway

@IgorDeepakM
Copy link
Contributor Author

Do we add an extra std/shell for stuff like quoteShell or should they go here?

Also you don't have to bother manually squashing & force pushing every time, it's done when merging anyway

quoteShell can go to a separate module like std/shell, which is possible. However, does that belong to this PR?

I tried to keep it clean on my local branch, that's why I used amend commits with force push. I was not sure how smart Github is when it comes to magically remove merge commits and subsequence change commits.

@metagn
Copy link
Collaborator

metagn commented Nov 29, 2022

Yes it's fine if another new module is not part of this PR.

@Araq Araq merged commit 84ea62e into nim-lang:devel Nov 30, 2022
@github-actions
Copy link
Contributor

Thanks for your hard work on this PR!
The lines below are statistics of the Nim compiler built from 84ea62e

Hint: mm: orc; opt: speed; options: -d:release
165399 lines; 8.711s; 614.41MiB peakmem

@rockcavera
Copy link
Contributor

I believe there is a changelog entry missing.

@metagn
Copy link
Collaborator

metagn commented Nov 30, 2022

I think the OS splits will have a change log entry at the end to avoid conflicts

ringabout added a commit that referenced this pull request Dec 2, 2022
@ringabout
Copy link
Member

ringabout commented Dec 2, 2022

I think the OS splits will have a change log entry at the end to avoid conflicts

@IgorDeepakM Nice work! It would be appreciated if you can add it to changelogs following

Added new modules which were part of std/os

Varriount added a commit that referenced this pull request Dec 2, 2022
* minor cleanup 

follow up #20946

* Update lib/std/cmdline.nim

* Update lib/std/cmdline.nim

Co-authored-by: Clay Sweetser <[email protected]>
@ringabout
Copy link
Member

=> #21039

survivorm pushed a commit to survivorm/Nim that referenced this pull request Feb 28, 2023
Command line paramater code moved from os.nim to cmdparam.nim

Co-authored-by: IgorDeepakM <[email protected]>
survivorm pushed a commit to survivorm/Nim that referenced this pull request Feb 28, 2023
* minor cleanup 

follow up nim-lang#20946

* Update lib/std/cmdline.nim

* Update lib/std/cmdline.nim

Co-authored-by: Clay Sweetser <[email protected]>
capocasa pushed a commit to capocasa/Nim that referenced this pull request Mar 31, 2023
Command line paramater code moved from os.nim to cmdparam.nim

Co-authored-by: IgorDeepakM <[email protected]>
capocasa pushed a commit to capocasa/Nim that referenced this pull request Mar 31, 2023
* minor cleanup 

follow up nim-lang#20946

* Update lib/std/cmdline.nim

* Update lib/std/cmdline.nim

Co-authored-by: Clay Sweetser <[email protected]>
bung87 pushed a commit to bung87/Nim that referenced this pull request Jul 29, 2023
Command line paramater code moved from os.nim to cmdparam.nim

Co-authored-by: IgorDeepakM <[email protected]>
bung87 pushed a commit to bung87/Nim that referenced this pull request Jul 29, 2023
* minor cleanup 

follow up nim-lang#20946

* Update lib/std/cmdline.nim

* Update lib/std/cmdline.nim

Co-authored-by: Clay Sweetser <[email protected]>
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.

5 participants