Fix #240: Avoid duplicated list of facilities #290
Conversation
ghost
added
the
|
Changes Unknown when pulling 5c864a1 on fix/240-logging into ** on master**. |
|
Changes Unknown when pulling d63e931 on fix/240-logging into ** on master**. |
There was a problem hiding this comment.
Looks good. One comment presents an idea about organizing the repository that probably extends beyond the scope of this PR. If you agree with my thoughts there and want to implement those changes in this PR that's great, but if you disagree or would rather hold off I am good with merging this as it is.
logging.go
Outdated
|
|
||
| // LoggingFacilitiesExtra provides logging identifiers | ||
| // used in ipfs-cluster dependencies, which may be useful | ||
| // for to display. Along with their default value. |
There was a problem hiding this comment.
little typo: "for to display"
nodebug.go
Outdated
| @@ -2,14 +2,13 @@ | |||
|
|
|||
| package ipfscluster | |||
|
|
|||
| // This is our default logs levels | |||
| // These are our default log levels | |||
There was a problem hiding this comment.
A general comment about nodebug, silent, debug, and logging: is it possible to put them somewhere a little more cohesive than simply in the main ipfscluster directory? Until right now I hadn't put it together that all these files were part of the logging system and think it would aid people understanding the codebase to somehow group them better. I can believe right now that they need to be part of ipfscluster and each use init functions so as it stands they can't be put into a single file or put into a subfolder. Maybe it doesn't need to happen in this PR, but if you agree then maybe we could open an issue for this?
There was a problem hiding this comment.
Actually, I have realized that it is possible to pass flags to tests just like any executable. This nodebug etc files were just a hack to get such behaviour from long ago. I'll complete the PR getting rid of them.
There was a problem hiding this comment.
ok, how is looking now?
hsanjuan
force-pushed
the
fix/240-logging
branch
from
d63e931
to
87f1b57
Compare
This puts some sanity in this. It's not super correct (name of facilities depend of the component and the main cluster component should not hard code them), but it's clear enough. Imho, better than over-engineering a more elegant approach. License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>
hsanjuan
force-pushed
the
fix/240-logging
branch
from
6188339
to
9c0f893
Compare
|
Hmm I broke something, travis loops over and over the same tests |
License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>
hsanjuan
force-pushed
the
fix/240-logging
branch
from
e75a2ce
to
bfdd597
Compare
|
@ZenGround0 made changes to |
This puts some sanity in this. It's not super correct (name of facilities
depend of the component and the main cluster component should not hard
code them), but it's clear enough. Imho, better than over-engineering
a more elegant approach.
License: MIT
Signed-off-by: Hector Sanjuan code@hector.link