search

cheshirekow / cmake_format

Source code formatter for cmake listfiles.
GNU General Public License v3.0
954 stars 105 forks source link

Changes to use as a lib #198

Closed rpavlik closed 4 years ago

rpavlik commented 4 years ago

I had some Python code that was generating CMake scripts using Jinja2, and I wanted to run cmake-format on it automatically. The location of the process_file function in __main__ made it inaccessible to a normal consuming python app. This change makes it accessible, so that it can be automated.

rpavlik commented 4 years ago

Hopefully the CI passes now - I re-arranged my change to group the imports all together, and pushed again with the commit signed (couldn't tell if that was required to pass)

rpavlik commented 4 years ago

Ah, I see, once I found your contributing doc. A copyright assignment is needed.

I don't know if I could sign it even if I were inclined to (which I'm not, both on principle -- why use the GPL if you will require copyright assignment? the gpl protects contributors and a CA asks me to waive that protection -- and on pragmatism: it's a two-line change) since I'd have to run it through my employer. You're free to accept this under the GPL-3, on the same terms the code was offered to me (incoming == outgoing).

Hopefully you reconsider your copyright assignment policy since it's a sizable roadblock to contribution: looks like all the "broken CI" pull requests currently open are broken at least in part because of something CA-related. (Looking at your closed pull requests - nearly all are closed without merging. I'm not sure if that's because of the CA or because of the "glue everything together in one commit per release" policy, but it looks discouraging.) I hadn't submitted this earlier despite doing this months ago and couldn't remember why - now I remember.

Good luck, and thanks for your work on this project!

cheshirekow commented 4 years ago

since I'd have to run it through my employer

If you're employer is OK with you contributing under GPL seems like they'd be fine with you contributing under a CA where that contribution is in-turn provided under GPL. But if not:

If you’d like to make a significant contribution to cmake-format but don’t agree to the terms of the copyright assignment please contact us to set up an alternate agreement. Otherwise, please consider filing a feature-request for changes you would like to see implemented.

Ideally, if there's a feature you need, I'd be happy to see it done here so you don't have to deal with the maintenance of constantly rebasing your fork.

And on to the technical feedback, I'm not sure why this change is needed. You can already import these two names from their respective modules. What is the purpose of this change?

rpavlik commented 4 years ago

I'm pretty sure you can't import from a double underscore module from outside: think I had to do this to get access to the thing in __main__

cheshirekow commented 4 years ago

I'm pretty sure you can't import from a double underscore module from outside: think I had to do this to get access to the thing in __main__

Really? I've never heard of this. cmake-lint imports cmake_format.__main__ (source here) as does the test-suite for cmake_format (source here). I import __main__ from ipython often for testing/debugging. Is this a new restriction in a recent python?

rpavlik commented 4 years ago

Hmm. I could have sworn I tried this, but I tried it again and it worked fine. Ignore this PR then, thanks for pointing this out!