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.

Sign up for GitHub

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

Jump to bottom

Deprecate readWriteThis methods #19983

Merged
merged 16 commits into from
Jun 15, 2022
Merged

Deprecate readWriteThis methods #19983

merged 16 commits into from
Jun 15, 2022

Conversation

benharsh
Copy link
Member

@benharsh benharsh commented Jun 13, 2022 •
edited
Loading

This PR updates various modules and tests to use readThis and writeThis instead of readWriteThis methods. Existing methods that weren't no-doc'd are left in place and given their own deprecation warning.

This PR also adds a new future for reading a map: test/library/standard/Map/testMapIO.future

Testing:

  • full local
  • gasnet

TODO:

  • Open issue for chpldoc bug
  • Open issue for new map future
  • Update docs with deprecation warnings (e.g. specialMethods primer)

@benharsh benharsh force-pushed the deprecate-readWriteThis branch from 84bebf0 to 7df7faf Compare June 13, 2022 22:43
@cassella
Copy link
Contributor

doc/rst/primers/specialMethods.rst has a section that covers and sort of recommends writing a readWriteThis(). Should that be updated?

@benharsh benharsh mentioned this pull request Jun 14, 2022
Closed
7 tasks
lydia-duncan
lydia-duncan approved these changes Jun 14, 2022
View reviewed changes
Copy link
Member

@lydia-duncan lydia-duncan left a comment

Choose a reason for hiding this comment

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

Really minor suggestions, otherwise this looks good! Thanks for taking it on!

modules/packages/TOML.chpl
compilerError("Reading a Tokens type is not supported");
}

proc writeThis(f) throws {
// TODO: The `list` type currently doesn't support readWriteThis!
Copy link
Member

Choose a reason for hiding this comment

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

Should this comment also be updated?

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 think it's mainly referring to the <~> operator there. I can update it in the other PR.

@benharsh benharsh force-pushed the deprecate-readWriteThis branch from 7df7faf to f3a18d6 Compare June 14, 2022 23:12
@benharsh benharsh mentioned this pull request Jun 14, 2022
Closed
@benharsh benharsh force-pushed the deprecate-readWriteThis branch from f3a18d6 to 0ccf2ec Compare June 14, 2022 23:39
@cassella
Copy link
Contributor

doc/rst/primers/specialMethods.rst has a section that covers and sort of recommends writing a readWriteThis()

Oh, sorry. I'd been skimming too quickly, and was only looking for the doc/rst/primers path that I had locally (that I guess came from a make docs a while ago), and didn't see you'd already adjusted the source file in test/release/examples/primers. (My comment probably sounded awfully opaque since you'd already made the change I was trying to describe.) Thanks for taking my advice anyway! 😂

benharsh added 16 commits June 14, 2022 20:12
@benharsh
Add compiler warning for 'readWriteThis' methods
4f3b8c2 
Signed-off-by: Ben Harshbarger <ben.harshb@gmail.com>
@benharsh
Update internal types to no longer use readWriteThis
377cda4 
Signed-off-by: Ben Harshbarger <ben.harshb@gmail.com>
@benharsh
Update tuple type to use readThis and writeThis
b1de950 
Signed-off-by: Ben Harshbarger <ben.harshb@gmail.com>
@benharsh
Update map type to use readThis/writeThis, and deprecate the document…
5a6f44b 
…ed readWriteThis method

Signed-off-by: Ben Harshbarger <ben.harshb@gmail.com>
@benharsh
Update DateTime types to use readThis/writeThis, document the expecte…
e59cbd5 
…d formats, and deprecate readWriteThis methods

Signed-off-by: Ben Harshbarger <ben.harshb@gmail.com>
@benharsh
Updates package module types to use readThis/writeThis
f9a16be 
These types didn't actually support reading because the compiler would
have issued an error for the use of string literals with the <~>
operator. Instead, issue a compilerError and no-doc the readThis
methods.

Signed-off-by: Ben Harshbarger <ben.harshb@gmail.com>
@benharsh
Add future for reading Map type
0975a5a 
Signed-off-by: Ben Harshbarger <ben.harshb@gmail.com>
@benharsh
Update tests to no longer use readWriteThis
c50c850 
Signed-off-by: Ben Harshbarger <ben.harshb@gmail.com>
@benharsh
Update readWriteThis deprecate text: or -> and
7a1e45d 
Signed-off-by: Ben Harshbarger <ben.harshb@gmail.com>
@benharsh
Remove undocumented readWriteThis
c2938ab 
Signed-off-by: Ben Harshbarger <ben.harshb@gmail.com>
@benharsh
Add workaround for chpldoc bug
3e8b70d 
Signed-off-by: Ben Harshbarger <ben.harshb@gmail.com>
@benharsh
Correct wording for map.readThis
471cc4e 
Signed-off-by: Ben Harshbarger <ben.harshb@gmail.com>
@benharsh
fix misc testing failures
6725a02 
Signed-off-by: Ben Harshbarger <ben.harshb@gmail.com>
@benharsh
Add deprecation warning to module-scope documentation for readWriteThis
e304a72 
Signed-off-by: Ben Harshbarger <ben.harshb@gmail.com>
@benharsh
Update primer to not mention 'readWriteThis'
4011949 
Signed-off-by: Ben Harshbarger <ben.harshb@gmail.com>
@benharsh
Add deprecation test for readWriteThis methods
1964692 
Signed-off-by: Ben Harshbarger <ben.harshb@gmail.com>
@benharsh benharsh force-pushed the deprecate-readWriteThis branch from 0ccf2ec to 1964692 Compare June 15, 2022 03:24
@benharsh benharsh merged commit f6eb173 into chapel-lang:main Jun 15, 2022
@benharsh benharsh mentioned this pull request Jun 16, 2022
Merged
benharsh added a commit that referenced this pull request Jun 16, 2022
@benharsh
Merge pull request #20015 from benharsh/fix-readwrite-test
8b365fe 
Fix read-write-locale-arg test to emit expected output

I made an error in rebasing #19983 and took the wrong set of changes.

Trivial, not reviewed.
@benharsh benharsh deleted the deprecate-readWriteThis branch August 7, 2022 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lydia-duncan lydia-duncan lydia-duncan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@benharsh @cassella @lydia-duncan