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

some minor modifications for include() (issue6056) #64

Merged
merged 2 commits into from
Nov 23, 2012

Conversation

fflorent
Copy link
Member

Takes into account Honza's remarks: http://code.google.com/p/fbug/issues/detail?id=6056#c30

I took the freedom to add others changes:

  • the return statement for commandLineInclude.js is now CommandLineInclude, that has more chance to be useful than CommandLineRep. Moreover, CommandLineRep can be shared if we store it in FirebugReps
  • in firebug.properties, I adapted commandline.include.loadFail
  • for commandline.include.wrongAliasArgument, the message is "Wrong alias name", that implicitly asks for a non-empty string.

Don't hesitate to tell me if you are OK with them or not.

Thanks Honza

Florent

@@ -804,11 +804,11 @@ commandline.include.tooLongAliasName=Alias names must not contain more than 30 c

# LOCALIZATION NOTE (commandline.include.wrongAliasArgument): For the include() function. This message is displayed
# in the Console panel if the provided alias name is invalid
commandline.include.wrongAliasArgument=Wrong alias argument; expected string.
commandline.include.wrongAliasArgument=Wrong alias name.
Copy link
Member

Choose a reason for hiding this comment

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

Not that it matters much, but I'm not sure I like this change. include("test", 1) doesn't as much have a "wrong name" as much as it doesn't have one at all (it has an integer). So I like the original better (maybe with "expected non-empty string" if it matters, and I'm not sure it does). Also, s/wrong/invalid; "wrong" makes me think in the direction of "you guessed the wrong password".

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, s/wrong/invalid

Good point. I change that (for WrongUrlArgument too).

include("test", 1) doesn't as much have a "wrong name" as much as it doesn't have one at all (it has an integer)

So why not: "Invalid second argument; expected alias name" ?

Copy link
Member

Choose a reason for hiding this comment

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

Works for me.

janodvarko added a commit that referenced this pull request Nov 23, 2012
some minor modifications for include() (issue6056)
@janodvarko janodvarko merged commit bf33eb2 into firebug:master Nov 23, 2012
@janodvarko
Copy link
Member

the return statement for commandLineInclude.js is now CommandLineInclude, that has more chance
to be useful than CommandLineRep.

Agree, I usually do the same. Modules should be the shared communication point.

Honza

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.

3 participants