refactor for toml no null #108
Conversation
| pages.append(Section(page["name"], page["icon"])) # type: ignore | ||
| pages.append( | ||
| Section(page["name"], page["icon"] if hasattr(page, "icon") else None) | ||
| ) # type: ignore | ||
| else: | ||
| pages.append(Page(**page)) # type: ignore |
There was a problem hiding this comment.
What do you think about handling "icon" here the same way you've handled it in the "if section" block above? That seems like it would nicely handle a lot of the other checks, and could simply default to null.
There was a problem hiding this comment.
sure, I will work on it tom and get back !
There was a problem hiding this comment.
I have made the changes to set icon to None when initializing here but I think the string null check has to be done separately when setting the page title and section names.
Let me know if that looks good!
…ng for page title and section name
for more information, see https://pre-commit.ci
|
I'm taking a look more at your solution, but I'm also struggling to understand exactly what the issues is -- if you simply leave the icon missing, it works fine as-is -- you can see the example with the You can see the result at https://st-pages.streamlit.app/ Is there some different behavior you're trying to support? |
|
I think the example app uses pages_sections When I use the current lib with the icon attr removed from the toml,it errors on line 202 as page[“icon”] is not found |
|
@Sairam90 it uses both, depending on whether the toggle for "Sections" it checked, but both of them have no icon for "example three" Ah, so the actual problem is if you have a Section with no icon. Yeah, that makes sense -- I didn't realize that was the problem. In that case, can you limit your changes to just support Sections with no icon? That would be great. Everything else should work fine as-is. |
src/st_pages/__init__.py
Outdated
| @@ -199,7 +201,9 @@ def _get_pages_from_config(path: str = ".streamlit/pages.toml") -> list[Page] | | |||
| for page in raw_pages: | |||
| if page.get("is_section"): | |||
| page["path"] = "" | |||
| pages.append(Section(page["name"], page["icon"])) # type: ignore | |||
| pages.append( | |||
| Section(page["name"], page["icon"] if hasattr(page, "icon") else None) | |||
There was a problem hiding this comment.
I think this should be the only required change.
There was a problem hiding this comment.
Ah..got it..cool..I will make the changes in some time!
There was a problem hiding this comment.
Done now ..thanks for the help!
There was a problem hiding this comment.
There was a problem hiding this comment.
-
The icon should not default to
'', but toNone-- are you sure it's defaulting to''? -
Yeah,
page.get("icon")should work well there
There was a problem hiding this comment.
Yes, cause I see the leading space issue in the section name alone in this case,though am not sure why it defaults to ‘’ instead of None
As we cannot assign None to an attribute in a toml file we must ignore the attribute 'icon' or set it to empty string to account for this
toml-lang/toml#975
Added a fix related to this - check if attr 'icon' is present in page object and if attribution 'icon' is not empty - add it to page title else ignore
@blackary - Could you please have a look - I tried using st-pages==1.0.1 and faced the above issue ; built the lib and used it locally and it works as expected