-
-
Notifications
You must be signed in to change notification settings - Fork 617
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
vstudio staticlib symbolspath #1033
vstudio staticlib symbolspath #1033
Conversation
modules/vstudio/vs2010_vcxproj.lua
Outdated
function m.programDatabaseFile(cfg) | ||
if cfg.symbolspath and cfg.symbols ~= p.OFF and cfg.debugformat ~= "c7" then | ||
m.element("ProgramDatabaseFile", nil, p.project.getrelative(cfg.project, cfg.symbolspath)) | ||
|
||
local key = iif(cfg.kind == p.STATICLIB, "ProgramDataBaseFileName", "ProgramDatabaseFile") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I would prefer this to be split into two methods, one for "ProgrameDatabaseFileName", and one for "ProgramDatabaseFile"..
While I get that they will be almost identical except for their name, it allows for easier override functionality...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was my first implementation but that would duplicate the condition and would be a pain to maintain IMO.
But I get your point about override, if you feel that's the way to go, I have no objection.
I will make the changes later today.
Thank you for all the added tests, much appreciated. |
Thanx.... if maintenance is a concern, then I guess you could make a local function for it, and we both have our way ;) local function dothatthing(name, cfg)
....
end
function m.programDatabaseFile(cfg)
dothatthing("ProgramDatabaseFile", cfg)
end
function m.programDatabaseFileName(cfg)
dothatthing("ProgramDataBaseFileName", cfg)
end Up to you if that's a change you would prefer to make, otherwise I'm happy with these changes, and someone can merge this. |
This solution sounds good to me, I will implement it later today when I have some time. |
ref: #1032
Also fixed names from tests I added in #1030