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

additional language support functions: BS_LINENUM(), BS_FILE(), BS_FUNCTION() #13

Closed
georgejecook opened this issue Jul 10, 2019 · 11 comments · Fixed by #126
Closed

additional language support functions: BS_LINENUM(), BS_FILE(), BS_FUNCTION() #13

georgejecook opened this issue Jul 10, 2019 · 11 comments · Fixed by #126

Comments

@georgejecook
Copy link
Contributor

georgejecook commented Jul 10, 2019

wouldn't it be great if we could drop these into the code at any time, like macros, so that they could be resolved at compile time. These are super handy for debugging, and currently require the use of third party tools.

Suggestion

print "unknown error occurred " ; BS_FILE() ; "." BS_FUNCTION() ; BS_LINENUM()

Would compile to
print "unknown error occurred pkg:/path/ToFile.brs.LoadStuff(23)"

@triwav
Copy link
Contributor

triwav commented Jul 10, 2019

The downside of this is you’d have to add it at the call site. If you have third party you can inject based off some regex for log statements which is easier (with a higher setup threshold) if this could be expanded that if it’s included as default value of a function it’s actually the line, function, file that called that function

@TwitchBronBron
Copy link
Member

TwitchBronBron commented Jul 10, 2019

BrightScript already supports LINE_NUM (Roku docs reference), so we don't need to add that. LINE_NUM is resolved at runtime as well, which is how it should work, in my opinion. I'm not a fan of injecting static line numbers at compile time, when 3d party preprocessors could potentially inject new lines/remove lines and potentially offset those static line numbers. Is there any benefit to having those line numbers be static?

I'm definitely open to adding constants for file name and function name though. How about following the same naming convention as LINE_NUM?

  • FILE_NAME
  • FILE_PATH
  • FUNCTION_NAME.

The one question I have is, what should we show for FUNCTION_NAME for unnamed functions (see my setTimeout example below)?

Syntax

function Main()
    print LINE_NUM
    print FUNCTION_NAME
    print FILE_PATH
    print FILE_NAME
    
    person = {
        sayHello: function()
            print FUNCTION_NAME
        end function
    }

    setTimeout(function()
        print function_name
    end function, 100)
end function

Output

function Main()
    print LINE_NUM ' no change because this is resolved at runtime by Roku
    print "Main"
    print "pkg:/source/Main.brs"
    print "Main.brs"
    
    person = {
        sayHello: function()
            print "sayHello"
        end function
    }

    setTimeout(function()
        print "anonymous" 'should this be empty? Say "<anonymous>"? 
    end function, 100)
end function

@triwav
Copy link
Contributor

triwav commented Jul 10, 2019

So the main issue is that the built in won’t give the line it was called from which again is what I want. Perhaps something like this that requires the files not be modified after could go in another step in the pipeline right before zip but after our callback to allow the user to make changes.

@georgejecook
Copy link
Contributor Author

I agree; I didn't specifically think of this feature for me though - there are (unfortunately) many who don't even consider a proper logging solution, who will benefit from this.

Also agree on your changes @TwitchBronBron I didn't suggest those as I was worried about naming collisions with existing code; but if you are fine with it, I prefer the consistency too

@TwitchBronBron
Copy link
Member

@triwav, are you saying you want the line number of the source brightscript file, NOT the line number of the actual runtime line number?

@triwav
Copy link
Contributor

triwav commented Jul 11, 2019

@TwitchBronBron What I actually want is say something like this in fileB at line 20:
logWarn(message as String, ..., fileName = FILE_NAME as String, functionName = FUNCTION_NAME as String, lineNumber = LINE_NUM as Integer)

to when called from fileA at line 10 in init() to return fileName as fileA, line number as 10 and function name as init

@TwitchBronBron
Copy link
Member

@triwav I believe @georgejecook is asking for current line number, file name, function name, etc. What you're asking for is previous information. , so perhaps create a separate github issue to track this, since what you're asking for is quite a bit more difficult. We would need to implement some type of tracking or set a global variable for every scope, and update it before every function call.

@triwav
Copy link
Contributor

triwav commented Jul 19, 2019

@TwitchBronBron totally get that. At least in my logging setup the current discussion doesn't have much value to me so thought I'd chime. To be clear, everything with what I'm saying would be done when we convert from brighterscript to brightscript. It would basically take those placeholders and do the replacement at the call site instead of inside the function. Hope that makes sense.

@georgejecook
Copy link
Contributor Author

It seems to me that @TwitchBronBron is reading my original intent well. I wanted simply the line number, file and function for the current file. If we're asking for some kind of other (far more complicated, and much more involved) stack trace mechanism, then I think we should discuss that in it's own issue. Of course, I have opinions on that too; but I don't believe this issue is the place for them :)

@TwitchBronBron
Copy link
Member

@georgejecook just to be clear, you are not asking for source line number, right? You want the runtime line number? Do you see any value in supporting a SOURCE_LINE_NUMBER variable that statically inserts the line number of the current source file?

@georgejecook
Copy link
Contributor Author

oh - yes - the line number should be the line number in the file at processing time not at runtime (in case it changed due to code restructuring)

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 a pull request may close this issue.

3 participants