-
Notifications
You must be signed in to change notification settings - Fork 39
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
nimsuggest: clean up nimsuggest.nim.cfg
#901
nimsuggest: clean up nimsuggest.nim.cfg
#901
Conversation
The change looks good to me, but if you also remove the two disabled |
Since you've also removed the active |
updated. |
I'm a bit uneasy about the I think it'd be better to leave the conditional symbol be. |
no, they should use config's |
Yep, I agree. But for consistency with the other programs (compiler and If none should, then I think that's better changed as a separate PR. |
52cacfc
to
5a90343
Compare
updated. |
nimsuggest.nim.cfg
@bung87: I've updated the PR title and body. The removal of the disabled defines is self-explanatory, but removing the additional import path is not. In general, something being unused doesn't necessarily warrant its removal, so I added some elaboration as to why the import path should be removed. |
okay, thank you! this one make me re consider the description thing, as it's 3 lines removal from config file, I never thought it worth to write such long description, as they exists maybe just by arbitrarily copy paste. anyway I'll try to do better. |
Personally, I think the length of the PR message should be dependent on the impact and complexity of the change, not on the number of lines changed. Nonetheless, I also think that a description should not be lengthy just for the sake of being long. Take, for example, a hypothetical PR that removes all superfluous whitespace in a repository. If many of those exists, the changeset of said PR would be very large, possibly modifying each and every file. The impact on the code, architecture, future consideration, etc. would be very small, however, and I think a short description that includes a reason why superflous whitespace should be removed is enough. On the other hand, even a single-line change can have a profound impact on the code base and possibly invalidate many invariants, in which case a longer description detailing the reasoning, consequences, etc. is warranted/needed. |
/merge |
Merge requested by: @zerbina Contents after the first section break of the PR description has been removed and preserved below: |
Summary
Remove obsolete and disabled configuration options from the
nimsuggest.nim.cfg
configuration file.Details
lib/packages/docutils
to the import paths; importsin the compiler and tools should not (and do not) rely on extra
import paths besides the compiler root and library directory
booting
andnoDocgen
symbolsThe
useStdoutAsStdmsg
define, while also obsolete, is kept forconsistency with the compiler and
nimfix
, both which also definethis conditional symbol.