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

Lazy evaluation #71

Merged
merged 5 commits into from Jan 23, 2021
Merged

Lazy evaluation #71

merged 5 commits into from Jan 23, 2021

Conversation

mdellweg
Copy link
Member

No description provided.

@mdellweg mdellweg force-pushed the lazy_evaluation branch 3 times, most recently from c2b06c8 to fcf629c Compare January 19, 2021 13:49
@mdellweg mdellweg force-pushed the lazy_evaluation branch 3 times, most recently from b75f7d7 to 7078d33 Compare January 20, 2021 13:56
return super().create(parameters=parameters, body=body)

@property
def HREF(self) -> str: # type: ignore
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not proud of tricking the typing system here. Maybe we need to wrap the HREF in a more generally used property we can override here.

# Workaround for improperly rendered nested resource paths and weird HREF names
# https://github.com/pulp/pulpcore/pull/1066
return {}
return {PulpExporterContext.HREF: self.exporter["pulp_href"]}
Copy link
Member Author

Choose a reason for hiding this comment

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

@ggainey I know, this sounds like going back and forth on how we do the trickery here. But at creation time of this Context we do not want to ask the server yet for its plugins.

@mdellweg mdellweg marked this pull request as ready for review January 21, 2021 11:58
@mdellweg mdellweg force-pushed the lazy_evaluation branch 2 times, most recently from ef24b72 to d7db622 Compare January 21, 2021 12:01
@pulp pulp deleted a comment from pulpbot Jan 21, 2021
@pulpbot
Copy link
Member

pulpbot commented Jan 21, 2021

Attached issue: https://pulp.plan.io/issues/8118

@mdellweg
Copy link
Member Author

I added a small section to the README to explain the requirement for the help pages.

And there i think, this is ready for review @daviddavis, @ggainey, @dkliban

@mdellweg mdellweg mentioned this pull request Jan 22, 2021
README.md Outdated Show resolved Hide resolved
Explain how to perform code linting and testing, describe the need to
access help pages without server calls and improve the section for
version based workarounds.

[noissue]
@daviddavis
Copy link
Contributor

daviddavis commented Jan 22, 2021

The code here looks good to me. I'm not totally sure I totally understand the value of this feature though? Part of the reason I'm asking is that this would seem to preclude the future possiblity of populating our help text screens with information from the API. I see that you call this out in #8118. Do you have any thoughts on how we can accomplish both?

@mdellweg
Copy link
Member Author

The code here looks good to me. I'm not totally sure I totally understand the value of this feature though? Part of the reason I'm asking is that this would seem to preclude the future possiblity of populating our help text screens with information from the API. I see that you call this out in #8118. Do you have any thoughts on how we can accomplish both?

The goal is to be able to access all commands help screens without the need to provide a pulp server. Also I think taking the docstrings from the API might be undesireable, because they describe certain API calls in a level of detail that might be unappropriate for the corresponding cli command. An example can be found in this discussion:
#63 (comment)

@daviddavis
Copy link
Contributor

daviddavis commented Jan 22, 2021

Thank you. I agree that the API text is not always what we want to display in the CLI's help screens. I assume the direction you're proposing is that we write and maintain our own set of help text in the CLI?

We had this same sort of design in Pulp 2 and there were a lot of gaps in the CLI compared to the API. When I look at say the file remote creation endpoint compared to the CLI command (which has just url and name), there are a lot of options and help text there we ought to display but aren't. I do think this is a problem we ought to solve (how to make it easier to define help text) but I'm willing to accept that it's perhaps a future one.

I think this over more this weekend and perhaps @ggainey and/or @dkliban want to weigh in as well?

@mdellweg
Copy link
Member Author

I should add, that the code itself does not rule it out, but the policy that "a help screen must not call out to a server" does.
OTOH i can vividly imagine us hacking into click's help information gathering and extend it to incorporate boilerplate text modules generated by / with the context modules.

Think "Name of the {entity_type}.".format(entity_ctx.ENTITY).

@mdellweg mdellweg merged commit f140ccd into pulp:develop Jan 23, 2021
@mdellweg mdellweg deleted the lazy_evaluation branch January 23, 2021 16:36
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.

None yet

3 participants