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

Update to nim 0.10.2 coding convention #81

Merged
merged 5 commits into from
Jan 4, 2015
Merged

Update to nim 0.10.2 coding convention #81

merged 5 commits into from
Jan 4, 2015

Conversation

lou15b
Copy link
Contributor

@lou15b lou15b commented Dec 31, 2014

My first open source contribution - basically a bit of nimfix-type housekeeping intended to free your time for more important stuff:

  • Replaced use of deprecated library types, etc. with corresponding current ones
  • Converted types in the code from the old T/P convention to the new one
  • Changed the fist char of assorted enum values from upper case to lower
  • Moved ENimble (renamed to NimbleError) into its own file (nimbletypes.nim), to prevent problems with recursive imports that I experienced when compiling individual files.

Hope it suits.

@lou15b
Copy link
Contributor Author

lou15b commented Jan 1, 2015

Forgot to mention - I manually ran the test cases in tester.nim and they all passed. The comments in tester.nim indicate that execCmdEx doesn't return results in the correct order for one of those tests. Currently digging to see what the cause might be.

@lou15b
Copy link
Contributor Author

lou15b commented Jan 2, 2015

Found the unit test problem - out-of-sequence caused by output from stderr being flushed to stdout without first flushing stdout. Fixed by changing the failure test cases to look for the error message in stderr output only.

@dom96
Copy link
Collaborator

dom96 commented Jan 3, 2015

I'd rather not have yet another module just for NimbleError. Which modules were causing recursive import problems?

TVersionRange* = object
case kind*: TVersionRangeEnum
VersionRangeRef* = ref VersionRange
VersionRange* = object
Copy link
Collaborator

Choose a reason for hiding this comment

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

You shouldn't make the non-ref type accessible as it is not used anywhere from what I can see and remember. So we should just have a VersionRange.

@dom96
Copy link
Collaborator

dom96 commented Jan 3, 2015

Just some small things. Good job overall!

@lou15b
Copy link
Contributor Author

lou15b commented Jan 4, 2015

Thanks for your comments.

Sorry for the lack of descriptive comments in the push - I'm still a bit new to git and ended up including code updates with the merge from upstream. Here is a summary:

  • Merged the latest update
  • Incorporated your comment re VersionRange: replaced public VersionRange with private VersionRangeObj and replaced VersionRangeRef with public VersionRange. Hope I got the naming convention right - I'm a bit confused about Xtype vs XtypeRef vs XtypeObj :)
  • deleted binary file "version" - perhaps a compiled form of version.nim that was accidentally committed?

I understand your concern about NimbleError being in its "own" module. However, if I leave it in its original place (in tools.nim) then the following will not compile individually without first having compiled packageinfo.nim:

  • config.nim
  • tools.nim

The compiler error message is:
packageinfo.nim(63, 23) Error: undeclared identifier: 'NimbleError'

I also found that I could break the compile of download.nim by changing the order of its imports so that tools is imported before packageinfo. Interestingly, the same did not occur for nimble.nim - it compiled successfully regardless of whether tools was imported before packageinfo.

I suspect that you will probably find the same thing with the current ENimble (using nim v0.10.2).

Perhaps this is just a corner case, but I'm a bit fussy that way. I created nimbletypes.nim mainly because I couldn't think of a better (existing) place to put NimbleError, and I thought that it might be useful in the future to hold other such types that are common among modules. Anyway, if there's another place you'd rather have NimbleError I'll be happy to move it there.

@dom96
Copy link
Collaborator

dom96 commented Jan 4, 2015

That's fine. Thank you!

dom96 added a commit that referenced this pull request Jan 4, 2015
Update to nim 0.10.2 coding convention
@dom96 dom96 merged commit 4130771 into nim-lang:master Jan 4, 2015
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.

2 participants