Fix link of uid via icalendar.attr.uid_property import

[stevepiercy]

• Closes [BUG] Cross-component links are broken sometimes #994

Description

Resolve broken cross-reference links in new() methods.

Checklist

• I've added a change log entry to CHANGES.rst.
• I've added or updated tests if applicable.
• I've run and ensured all tests pass locally by following Run tests.
• I've added or edited documentation, both as docstrings to be rendered in the API documentation and narrative documentation, as necessary.

[coveralls]

Pull Request Test Coverage Report for Build 20748744326

Details

• 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 97.816%

---

Totals
Change from base Build 20748733697:	0.0%
Covered Lines:	10416
Relevant Lines:	10633

---

💛 - Coveralls

[niccokunzmann]

Looks like a fix. Is uid part of the superclass Component?

[niccokunzmann]

Ah, yes. For this uid, it would be this:

:attr:`~icalendar.Component.uid`

because of

class Component....

    uid = uid_property
    ....

The reason for the inconsistency here is that some properties are defined on all components according to later RFCs but RFC5545 for example only defines them on certain components. That is why there are not present in all Components.

[stevepiercy]

I found this, but I don't know whether that makes it 🦸 . hehe.

icalendar/src/icalendar/cal/component.py

Lines 9 to 20 in 6a8e974

	from icalendar.attr import (
	CONCEPTS_TYPE_SETTER,
	LINKS_TYPE_SETTER,
	RELATED_TO_TYPE_SETTER,
	comments_property,
	concepts_property,
	links_property,
	refids_property,
	related_to_property,
	single_utc_property,
	uid_property,
	)

icalendar/src/icalendar/cal/component.py

Line 688 in 6a8e974

uid = uid_property

For the other attrs that don't link, should we do a similar treatment?

• comments
• components
• concepts
• created
• last_modified
• links
• refids
• related_to
• stamp

Also this issue turns out to be less about documentation and more about code and feature compliance with the RFCs. I get concerned about tests as well. I'd prefer to focus on documentation, and let a first-timer pick this up by giving enough direction for them to run with it.

[stevepiercy]

It looks like we had crossed GitHub comments, and I just noticed you opened #1066.

The syntax you suggested doesn't work. The target must always resolve to a fully qualified dotted Python path.

For this one, I'd go with markup that indicates in which class uid resides to disambiguate the context. If it displays only uid, then it can mislead the reader to think it's in the class Availability.

:attr:`icalendar.cal.component.Component.uid`

or

:attr:`Component.uid <icalendar.cal.component.Component.uid>`

At this point, I'm not sure how to proceed. I pushed a commit so you can see the change.

[niccokunzmann]

I think, just displaying uid is fine from an OOP standpoint as uid is an attribute of the Availability class. I would not do the extra import but I don't mind. Only the documentation link needs fixing. You choose how you like to write it.

[stevepiercy]

I tried removing both the import and the variable that used the import. As a result, the only way that uid links is to use:

:attr:`Component.uid <icalendar.cal.component.Component.uid>`

...which links to https://icalendar.readthedocs.io/en/latest/reference/api/icalendar.cal.component.html#icalendar.cal.component.Component.uid

But it sounds like that's not what you want, and that you prefer it would link to:

https://icalendar.readthedocs.io/en/latest/reference/api/icalendar.cal.component.html#icalendar.cal.availability.Availability.uid

...which only exists when there is the import and variable that uses the import.

---

I don't have a preference for how to write something, other than the link and its label must align and have the proper context. If I were instead to do something like this in the Availability class:

:attr:`~icalendar.cal.component.Component.uid`

Then the link would go to a different context, outside of Availability and into Component. That's not aligned and would confuse readers.

[stevepiercy]

@niccokunzmann I pushed another revision. This links within the page, retaining context, and uses the short form. However the import and variable declaration are required to make the link work. Without these two lines, you have no Availability.uid object to link to.

without import

https://icalendar.readthedocs.io/en/latest/reference/api/icalendar.cal.availability.html#icalendar.cal.availability.Availability.uid

with import

https://icalendar--1064.org.readthedocs.build/en/1064/reference/api/icalendar.cal.availability.html#icalendar.cal.availability.Availability.uid

You can also look at the right-hand in-page navigation to see what I'm talking about.

[niccokunzmann]

Hm. I don't mind either way. One looks nicer and the other one is compact in code. They are both functionally valid. So, please choose how you like the docs.

I start thinking that it is more compact then to rename these properties if they have multiple imports so they can be imported in the class like from icalendar.cal.properties import uid.
If all these attributes are actually on the page of the respective component that would be quite nice for the reader, right? Docs is important and that might be quite good. What do you think?

What about consistency and other methods like to_ical? A style can be tried out here and then an issue created for first timers to make it consistent across all classes.

Is there a way some inherited functions and atrributes can be displayed? Is that desirable?

[stevepiercy]

Hm. I don't mind either way. One looks nicer and the other one is compact in code. They are both functionally valid. So, please choose how you like the docs.

Just to be clear, I'm saying that these two hyperlinks are not functionally valid as hyperlinks after Sphinx renders it to HTML. As Python code, they may be functionally valid, either with or without the two lines of the import and the variable assignment.

• "Without import" goes to current development docs without any Availability.uid in the right-hand navigation or in the page.

  

• "With import" goes to pull request preview docs with the Availability.uid in the right-hand navigation and in the page.

  

When there's a change in code, I don't think this is a personal preference, but an intentional design decision. That goes beyond mere appearance. I don't feel it's right for me to make a unilateral choice here.

I start thinking that it is more compact then to rename these properties if they have multiple imports so they can be imported in the class like from icalendar.cal.properties import uid. If all these attributes are actually on the page of the respective component that would be quite nice for the reader, right? Docs is important and that might be quite good. What do you think?

I think it's confusing to write reStructuredText roles for Python objects in icalendar.

To back up a bit, I first noticed this when I resolved all the "duplicate object" warnings emitted by Sphinx in a separate recent PR. Even Sphinx was confused, ha! That has been cleared up, specifically by always using a full dotted Python path in the target. That leaves us with the question of what should be the actual target, either the superclass or subclass?

I don't feel qualified to answer that question by myself. I don't actually use or develop in icalendar, although I've absorbed a basic understanding from just writing docs. That's where your experience, and that of other users and developers, is extremely valuable.

I think it would be reasonable to say that if I were writing code, and I couldn't find Availability.uid in the docs, I would assume it's not possible to use. I'd first look for it in the in-page navigation, and find nothing. Then I'd search the page, and see no separate entry. I would find uid in the page, buried in the Availability.new method.

Now imagine doing the same thing for an attribute that does appear, such as busy_type. My brain notices there's a difference, but doesn't know why, then shrugs it off as something to come back to later.

This difference causes a small dissonance for me as a documentarian, but this may be acceptable to users and developers.

What about consistency and other methods like to_ical? A style can be tried out here and then an issue created for first timers to make it consistent across all classes.

Do you mean where to_ical in https://icalendar.readthedocs.io/en/latest/reference/api/icalendar.prop.html is applied across all relevant classes? That's really clear to me and doesn't short-circuit my brain.

Is there a way some inherited functions and atrributes can be displayed? Is that desirable?

I'm not sure what you have in mind. Do you envision something like a list of inherited objects in a separate section, some way of marking them inline where they appear in the page, or something else? Here are some options.

1. Try :inherited-members: and configure it in make apidoc.
2. Try Sphinx AutoAPI instead of apidoc and autodoc.
3. Add pretty pictures with https://www.sphinx-doc.org/en/master/usage/extensions/inheritance.html.
4. Customize the source .rst files to display whatever we want, but the trade-off would be the loss of automatic generation of the .rst files from Python source files via make apidoc.
5. Ask in the Python documentation Discord server, but I'd like to better understand what you seek when I present the question.

[niccokunzmann]

From my perspective, either way does not make much of a difference:

• import + add uid
• link to component
• use :inherited-members:
• Add pretty pictures with https://www.sphinx-doc.org/en/master/usage/extensions/inheritance.html

Sometimes, we can take up a lot of time to decide on things that are almost equal in outcome.

I would say, you choose the one that feels best for you. You look at it from the outside a bit more than I do. So, if it works for you, then perfect! Go for it :) When someone has an issue with it in the future, they can still change it.

[niccokunzmann]

I think what would work well for me is to document the members that Component also has. But not the members that come before that. Would that solve the issue with the linking then? Hm. Maybe, I try my luck too at some point.