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

Add new redirect app to provide a url shortener feature #228

Merged
merged 4 commits into from
Jan 8, 2024

Conversation

sebastian-aranda
Copy link
Contributor

This PR adds a new redirect app to provide something like an url shortener feature, for easier link manipulation and sharing.

Copy link
Member

@pkubanek pkubanek left a comment

Choose a reason for hiding this comment

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

If you can drop references to M1M3BumpTests, as M1M3BumpTestsRedirectTestCase, to RedirectTestCase etc. - I will look again then. Thanks

@pkubanek
Copy link
Member

pkubanek commented Jan 5, 2024

Well not necessary redirect only, but this view is for M1M3ForceActuator data - so something on this tune. Thanks

Copy link
Member

@pkubanek pkubanek left a comment

Choose a reason for hiding this comment

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

Please see the proposed changes. Changing to enums shall be useful, a bit faster, and less memory-intensive than strings.

Returns
-------
int
The random force actuator
Copy link
Member

Choose a reason for hiding this comment

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

Better: The random force actuator record. Also, data type isn't int, but ForceActuatorData from lsst.ts.xml.tables.m1m3.

return fa.get_index(index_type)


class CHRONOGRAF_SITES:
Copy link
Member

Choose a reason for hiding this comment

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

Could that be turned into enum? Like:

class CHRONOGRAF_SITES(enum.IntEnum):
SUMMIT = 1
USDF = 2

etc..?

see https://github.com/lsst-ts/ts_xml/blob/develop/python/lsst/ts/xml/enums/MTM1M3.py for ane example.

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'm a bit hesitant of doing this change. Having the class with a str hash gives me easier handling of the view as the site param is set as a str in the url (and then it is used on utils functions that needs the site param as a string), e.g:

get_site_url_domain("summit")

Whereas summit can be usdf, base or tucson (added in case of future extensions). If I make the change to an enumeration, I'll need to create another hash class to make the conversion from the site in string to an enum.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed internally with @pkubanek , an StrEnum would be handy here, although we need to update to python > 3.11. A TODO was added for this.

@sebastian-aranda sebastian-aranda merged commit c9039ec into develop Jan 8, 2024
2 checks passed
@sebastian-aranda sebastian-aranda deleted the tickets/DM-42373 branch January 8, 2024 16:21
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.

2 participants