Constant names coding style & Log samples violation

Crystal Contrib
1

Crystal coding style enforces constants to be screaming-cased. This is what GitBook says about it:

# **Constants**  are screaming-cased. For example:

LUCKY_NUMBERS     = [3, 7, 11]
DOCUMENTATION_URL = "http://crystal-lang.org/docs"

However, the API doc for Log class uses camel case to define a Log constant:

module DB
  # => Log constant violates screaming case
  Log = ::Log.for("db") 
end

Could you please elaborate on what is the best way to address this conflict? Relates to https://github.com/crystal-ameba/ameba/issues/148

2 Likes
2

I would say we can split constants in 2 categories:

  • magic numbers (e.g: 42, in SCREAMING_CASE)
  • kind of static object we’ll need in the current scope (e.g: Log instance, in UpperCamelCase)
3

Log is a class and the Log constants inside other types are to make it look like classes…
In the case of ameba, I would just make an exception for Log.

2 Likes
4

I just ran into this while updating https://github.com/luckyframework/dexter to use the new Log module.

I think maybe ameba should allow UpperCamelCase since Log is probably not the first or last thing to be used in this way. For example, in Avram we already have a different constant name because we use 2 loggers in the same class: QueryLog and FailedQueryLog.

So that’d be my vote. Allow by default, or have a setting that you can enable/disable for UpperCamel constants

1 Like
5

I did exactly that, allowed UpperCamelCase (or PascalCase). You can try it out in the latest Ameba (v0.12.1). Will think about the setting to turn it off in the future. Thanks!

1 Like
6

Awesome @veelanga! Going to update now!

7

Hey! I’ve pointed out this issue to @veelenga over Ameba.

Now I’m playing around with the Log notation, I’m not 100% sure it’s a great idea. Here an example of code which doesn’t compile:

require "log"
 
module Main
  extend self
  Log = ::Log.for(self)
 
  def main
	Log.builder.bind "*", :debug, Log::IOBackend.new
    Log.warn { "This is a warning" }
  end
end
 
Main.main

https://play.crystal-lang.org/#/r/8vrz

The Log object shadow the Log class. But the Log IS an object, not a class, and doesn’t provides access to subclasses and tutti quanti.
And this shadowing is in my opinion quite bad (plus the error message can confuse a beginner).

1 Like
8

I have similar concerns as I’ve been migrating to the new logger. There is a lot that I love about it, but the Log constant reassignment has been fairly confusing in terms of inheritance and what works and doesn’t. I never really know which log I’m working with and a lot of times I’ve ended up having to create differently named logs anyway (FailedQueryLog). I'm planning to put together a post with some thoughts on the new Log` and ways that we may want to change/add some things

1 Like
9

Isn’t it the idea to shadow things on purpose, so you don’t care about which Log type you are using?

And then… why is there a need to inherit Log?

10

Ah, nevermind, disregard my comment. I think the idea is to shadow things. But also the idea is not to configure log in places where you would use them for logging. That way there’s no conflict. And if there is, you can always do ::Log.builder.bind

11

@asterite this is a simplified example. I just want to point out two things:

  • The Log here is not a module but a constant, and can confuse the beginner 100% sure.
  • Error: undefined method 'builder' for Log is the current error message. This one is confusing too. I guess Error: undefined method 'builder' for Main::Log would definitely give more insight on what’s going on here :smile:

In this case, I feel like we’re making an exception in conventions we follow everywhere, just because it’s more convenient (and actually it’s not so much more convenient).

12

In this early days of the module I think is common for people to configure the logging and use the logging at the same time.

But as frameworks incorporate it, and after a configuration module appears, there will be less confusion since the usages will be mostly for declaring Log sources and emitting logs.

13

Just as a general note: There’s nothing forcing you to call your log handler Log. LOG will do as well, or anything else.

14

make ::Log.for a macro and define an inner class, so Log is not Constant.

15

and just call Log.for, no need ::

class Log
  class_getter sources : Array(String)
  @@sources = [File.basename(PROGRAM_NAME)] of String

  macro for(source)
    class Log < ::Log
      class_getter sources : Array(String)
      @@sources = ::{{@type}}.sources + [{{source.id.stringify}}]
    end
  end

  def self.info(message)
    puts "[" + sources.join(":") + "]: #{message}"
  end
end

Log.info("top")

module X
  Log.for("x")

  def self.y
    Log.info("x")
  end

  class Z
    Log.for("z")

    def self.z
      Log.info("z")
    end

    class A
      Log.for("a")

      def a
        Log.info("a")
      end
    end
  end
end

X.y
X::Z.z
X::Z::A.new.a
16

but with a limit, can not do it inside a method.