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

Designate pants.ini as legacy, but still supported, in docs #9194

Merged
merged 5 commits into from
Mar 4, 2020

Conversation

Eric-Arellano
Copy link
Contributor

@Eric-Arellano Eric-Arellano commented Feb 27, 2020

We decided to not (yet) deprecate pants.ini because it causes too much friction for users at the same time that they're dealing with deprecating globs() and --test-fast. Further, the code to support pants.ini is not very painful to support as it is well-isolated (although, it would allow us to simplify things if we could always assume TOML).

But, we don't want to have to refer to both TOML and INI in our docs and user messages. So, we strike a compromise where we always assume TOML in our docs, but allow our code to work with both.

To reflect this decision, we update our docs to have a section explaining how INI works and pointing to a script to migrate to TOML. That is the only section in our docs that we should ever mention INI.

Copy link
Contributor Author

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

(Added so many of you all because this PR is mostly a discussion and decision for the project; not really code review.)

src/python/pants/option/config.py Outdated Show resolved Hide resolved
@cosmicexplorer
Copy link
Contributor

Have we discussed this change with pants-devel?

@blorente
Copy link
Contributor

I was talking about this with @olafurpg the other day, and we reasoned that pants.ini is hardcoded in so many systems at Twitter that it would be extremely hard for us to migrate Twitter off of it.

While I think toml is the better alternative, I don't think we can realistically stop using pants.ini in a few release cycles, and we'd need this to continue existing as an alternative. This is just my opinion though, @stuhood 's or @illicitonion 's might differ.

@Eric-Arellano
Copy link
Contributor Author

Have we discussed this change with pants-devel?

Not yet, because I wanted to get all of your thoughts first. If there were any doubts on whether we should do this (as it turns out there are) and we couldn't reach a decision, reaching out to pants-devel would be a great idea to get more user feedback.

I was talking about this with @olafurpg the other day, and we reasoned that pants.ini is hardcoded in so many systems at Twitter that it would be extremely hard for us to migrate Twitter off of it.

While I think toml is the better alternative, I don't think we can realistically stop using pants.ini in a few release cycles, and we'd need this to continue existing as an alternative.

Good to know!

I propose, then, that we keep support for pants.ini for a much longer time (possibly forever?). The code is not very expensive to support both formats. We'd be able to simplify if we dropped INI, but that code is 100% abstracted away in config.py and parser.py so the deprecation isn't as much a benefit in terms of code as something like removing globs().

Rather, the expense of supporting both formats is in documentation. It's difficult/clunky for us to refer to both pants.toml and pants.ini everywhere.

So, the proposal is that we keep support for INI, but mark pants.ini as something like "Legacy INI file format" and use TOML everywhere in our documentation, such as the first time install guide and config snippets we put in deprecation warnings. The only place we'd refer to it is in options.md on a section called "Legacy INI file format" which explains the differences from TOML.

--

If we agree to this plan, the next steps would be:

  1. Add "Legacy INI file format" section to options.md.
  2. Send an email to pants-devel explaining that we encourage them to migrate to pants.toml, but it's their choice. They can use either or even intermix the two.
    • Long term, maybe we encourage an incremental migration thanks to being able to intermix the two?

@blorente
Copy link
Contributor

I like this change a lot more, thanks a lot for reaching out @Eric-Arellano

# Delete this line to force a full CI run for documentation-only changes.
[ci skip]  # Documentation-only change.
@Eric-Arellano Eric-Arellano changed the title Deprecate pants.ini in favor of pants.toml Designate pants.ini as legacy (but still supported) in docs Mar 3, 2020
@Eric-Arellano Eric-Arellano changed the title Designate pants.ini as legacy (but still supported) in docs Designate pants.ini as legacy, but still supported, in docs Mar 3, 2020
@Eric-Arellano
Copy link
Contributor Author

As decided above, this PR is now updated to no longer deprecate INI, but to only mark it as legacy in our docs. This is ready for review :)

Copy link
Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

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

Thank you!

I was talking about this with @olafurpg the other day, and we reasoned that pants.ini is hardcoded in so many systems at Twitter that it would be extremely hard for us to migrate Twitter off of it.
FWIW, parsing pants.ini has never been a suggested approaching for external integrations... ./pants options supports rendering options as JSON, and IDE integrations should prefer that. cc @wisechengyi

Having said that, I agree that having breathing room to make that migration would be a very good idea, so thank you for bumping this out a bit.

# Delete this line to force a full CI run for documentation-only changes.
[ci skip]  # Documentation-only change.
@Eric-Arellano Eric-Arellano merged commit 7e6de3c into pantsbuild:master Mar 4, 2020
@Eric-Arellano Eric-Arellano deleted the deprecate-pants-ini branch March 4, 2020 14:19
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.

4 participants