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

Rename hy model types #2031

Merged
merged 7 commits into from
Apr 6, 2021
Merged

Conversation

peaceamongworlds
Copy link
Contributor

@peaceamongworlds peaceamongworlds commented Apr 1, 2021

Closes #1644.

All hy models have Hy removed from their name, so HyString -> String, HyList -> List etc.

Also, hy.models has been prepended to the repr of all the models.

There are still objects which include Hy in their name, such as all the hy errors, HyASTCompiler, HyREPL and maybe some others. It would be nice to rename those as well, if everyone agrees.

@Kodiologist
Copy link
Member

I'm against this because I'd rather skip right to hy.models.Integer, as I proposed in #1992, than make that change later and break people's code again.

@peaceamongworlds
Copy link
Contributor Author

I'm against this because I'd rather skip right to hy.models.Integer, as I proposed in #1992, than make that change later and break people's code again.

Are you opposed to including the models in the hy namespace, or changing their __repr__ methods?

You mentioned #1644 in #1992, but that suggests making the models accessible as hy.Model. quote already uses the fully qualified name (since #1979 I think), so changing their string representation seems like a more of an aesthetic change.

@Kodiologist
Copy link
Member

Are you opposed to including the models in the hy namespace, or changing their __repr__ methods?

Both. I want HyInteger to be accessible as hy.models.Integer and I want the repr to reflect that.

@peaceamongworlds
Copy link
Contributor Author

Fair enough

@peaceamongworlds
Copy link
Contributor Author

I've changed it so that the hy models can only be accessed through hy.models and repr shows hy.models as well.

@Kodiologist
Copy link
Member

Thanks. Could you take any changes that aren't part of the massive search-and-replaces, like the NEWS line, and put them in one or more commits separate from the search-and-replaces?

``hy.models.HySequence`` is the abstract base class of "iterable" Hy
models, such as HyExpression and HyList.
``hy.models.Sequence`` is the abstract base class of "iterable" Hy
models, such as hy.Expression and hy.List.
Copy link
Member

Choose a reason for hiding this comment

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

Looks like there are still a few places in the docs that aren't scoped to hy.models, possibly other locations as well.

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 think I forgot to check everywhere, but that should be fixed now.

@peaceamongworlds peaceamongworlds force-pushed the rename_hy_model_types branch 2 times, most recently from 600e725 to 644f507 Compare April 2, 2021 09:42
@peaceamongworlds
Copy link
Contributor Author

I think that should be everything. The commits are somewhat intertwined, but I've separated them out into logical sections.

Copy link
Member

@Kodiologist Kodiologist left a comment

Choose a reason for hiding this comment

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

Well done.

@scauligi scauligi merged commit 19f943d into hylang:master Apr 6, 2021
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.

Don't include the namespace in the import's name for hy models.
3 participants