search

alliedmodders / ambuild

AlliedModders C++ Build System
BSD 3-Clause "New" or "Revised" License
61 stars 32 forks source link

AMBuild API 2.1 fixes #73

Closed WildCard65 closed 4 years ago

WildCard65 commented 4 years ago

getattr(obj, 'rvalue', None) -- Expando has no "get" attribute method.

Changes to paths.IsSubPath -- Previous implementation raises a false negative under certain values.

NOTE: This is a repaired version of #42, pulling and moving commits from upstream resulted in broken PR.

dvander commented 4 years ago

The rest of this change looks good. I'd like to begin work on a 2.2 API (needed for some upcoming features), and that'll require forking the 2.1 API. And before that, I'd like to re-lint the entire project to use 4-space indents. I'm finding it really hard to read the code right now.

I'll make sure this gets merged before all that work takes place. If necessary I can fix the Windows path issue above and make sure this gets landed.

WildCard65 commented 4 years ago

If you can fix the Windows path issue, go for it, otherwise I'll need to look for the best approach for detecting different roots and inject a short circuit before the relpath call.

On Mon., Aug. 10, 2020, 4:16 p.m. David Anderson, notifications@github.com wrote:

The rest of this change looks good. I'd like to begin work on a 2.2 API (needed for some upcoming features), and that'll require forking the 2.1 API. And before that, I'd like to re-lint the entire project to use 4-space indents. I'm finding it really hard to read the code right now.

I'll make sure this gets merged before all that work takes place. If necessary I can fix the Windows path issue above and make sure this gets landed.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/alliedmodders/ambuild/pull/73#issuecomment-671566508, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABGFQEZUZ7QUI7PL2LVGOYLSABISBANCNFSM4PD4UEZQ .

dvander commented 4 years ago

Thanks, I've submitted these changes as #81.