Skip to content

Simplify example to not use translate requirement #85

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

Merged
merged 2 commits into from
Nov 12, 2021

Conversation

Eric-Arellano
Copy link
Contributor

As explained in #84, we think it's valuable for this example repository to be simple. In particular, we are demoing Pants itself, not the underlying code.

This simplifies the project to use our own hardcoded translate.json, rather than the much more robust translate requirement. We still demonstrate the same things translate was showing for us:

  • third-party management
  • loading resource files

There's an added benefit that the repository is now much faster to build, especially on Apple Silicon where lxml must be compiled from source. Likewise, I no longer have to force Pants to use Py39+ for this to build on my M1! The default of 3.7+ now works.

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.

Note that because I deleted config.py and config_test.py, we no longer have an example of >1 test per directory...I'm not sure how much we care about that.

Preserving config.py was too complex imo. We already have an example of loading a resource file, I really don't think we want to demonstrate that >1 time.

Comment on lines +13 to +15
python_tests(
name="tests",
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I very much prefer the style of keeping it on one-line. But tailor generates it this way, so probably good to be consistent with that.

name="tests",
)

# This target teaches Pants about our JSON file, which allows for other targets to depend on it.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# This target teaches Pants about our JSON file, which allows for other targets to depend on it.
# This target teaches Pants about our JSON file, which allows other targets to depend on it.

# This target teaches Pants about our JSON file, which allows for other targets to depend on it.
resources(
name="translations",
sources=["translations.json"],
Copy link
Member

Choose a reason for hiding this comment

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

Maybe glob *.json? A bit shorter, and less fragile.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In 2.8, I will update this to use resource instead of resources

@Eric-Arellano Eric-Arellano merged commit aa17fef into pantsbuild:main Nov 12, 2021
@Eric-Arellano Eric-Arellano deleted the no-translate branch November 12, 2021 21:34
@Eric-Arellano Eric-Arellano mentioned this pull request Feb 14, 2022
Eric-Arellano added a commit to pantsbuild/example-django that referenced this pull request Mar 11, 2022
Per pantsbuild/example-python#85, our example repos should be as simple as feasible so that users aren't distracted.

Regex validation is a nifty feature, but it's not important to showing off Django.
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