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

Prevent users from editing filename property on classes #104

Closed
jgd10 opened this issue Jan 26, 2022 · 1 comment · Fixed by #107
Closed

Prevent users from editing filename property on classes #104

jgd10 opened this issue Jan 26, 2022 · 1 comment · Fixed by #107
Assignees

Comments

@jgd10
Copy link
Contributor

jgd10 commented Jan 26, 2022

Whilst working on #98 I have made explicit filename and pathlib_filename properties on classes that have the filename property. I kept a setter for filename because to remove it would be a breaking change.

Additionally, whilst working on the tests for these changes I notice that if an invalid filename is provided the class constructor will throw an error, but obviously this wasn't happening with the setters then and it isn't happening with the setters I've added now. Do we want people to be able to set the filename like this? This would be on the Result class or Archive class, for example.

It would be trivial to delete the new setter methods before making my PR. Or alternatively I could add a check to the setters? However, this would still be a breaking change since this wasn't in the original behaviour. I would suggest just deleting the setters as it seems like you get undefined behaviour otherwise and the filename would still be accessible by the _filename property anyway if a user was particularly determined.

What do you think @akaszynski ?

Classes this affects:

  • EmatFile
  • FullFile
  • Archive
  • Result
@jgd10 jgd10 self-assigned this Jan 26, 2022
@akaszynski
Copy link
Collaborator

It would be trivial to delete the new setter methods before making my PR. Or alternatively I could add a check to the setters? However, this would still be a breaking change since this wasn't in the original behaviour. I would suggest just deleting the setters as it seems like you get undefined behaviour otherwise and the filename would still be accessible by the _filename property anyway if a user was particularly determined.

I think deleting the setters is the most straightforward approach.

jgd10 added a commit that referenced this issue Jan 31, 2022
This fixes #104. Removed the newly introduced filename setters and
amended docstrings to indicate the properties are read-only.
Additionally amended the tests to check the properties ARE read only
instead of checking that they were write as well.
akaszynski pushed a commit that referenced this issue Feb 1, 2022
This fixes #104. Removed the newly introduced filename setters and
amended docstrings to indicate the properties are read-only.
Additionally amended the tests to check the properties ARE read only
instead of checking that they were write as well.
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 a pull request may close this issue.

2 participants