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

Remove debug info from end of an error message (#7248) #7670

Merged
merged 1 commit into from
Oct 6, 2021
Merged

Remove debug info from end of an error message (#7248) #7670

merged 1 commit into from
Oct 6, 2021

Conversation

ivanperez-keera
Copy link
Contributor

The function that informs users that the package list is missing prints
an internal data structure at the end of the message, without formatting
it.

This extra info is likely just left over from debugging and does not
need to be printed.

This commit just removes that unnecessary show.

Please include the following checklist in your PR:

Please also shortly describe how you tested your change. Bonus points for added tests!

@ivanperez-keera
Copy link
Contributor Author

I believe what could belong in changelog.d would be a file called issue-7248 with the content:

synopsis: Error message about missing package list shows internal data structure RemoteRepo
packages: cabal-install                                                         
prs: #7670
issues: #7248

Is that correct? Do I do it or does someone else do it?

@ivanperez-keera
Copy link
Contributor Author

I'm happy adding it if that makes your life easier. I obviously didn't know the PR number yet, and didn't know if you preferred the file to be called issue-<number> or pr-<number>.

I looked at other PRs that people were putting in and did not see others adding those changelog.d files, so I did the same.

@ivanperez-keera ivanperez-keera mentioned this pull request Sep 20, 2021
Closed
@andreasabel andreasabel added the re: error-message Concerning error messages delivered to the user label Sep 20, 2021
@andreasabel andreasabel linked an issue Sep 20, 2021 that may be closed by this pull request
Error message about missing package list shows internal data structure RemoteRepo #7248
Closed
andreasabel
andreasabel approved these changes Sep 20, 2021
View reviewed changes
Copy link
Member

@andreasabel andreasabel left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Not sure a changelog entry is needed for this. It is rather a cosmetic change.

A golden value test for this error message would be nice to have.

However, I don't know if there is an infrastructure for such tests yet.
@fgaz and @Mikolaj might know.

@andreasabel andreasabel changed the title Remove debug info from end of error message (#7248) Remove debug info from end of an error message (#7248) Sep 20, 2021
@ivanperez-keera
Copy link
Contributor Author

I must confess I have not tested it. I only checked that the variable was still used in the scope so that it would not trigger a warning.

@fgaz
Copy link
Member

fgaz commented Sep 20, 2021 •
edited
Loading

I also think a changelog entry is unnecessary.

A golden test that tries to unpack a hackage package without making it available with withRepo can be added in cabal-testsuite

@Mikolaj
Copy link
Member

Mikolaj commented Sep 30, 2021

Would somebody volunteer to add the simple test?

andreasabel added a commit to andreasabel/cabal that referenced this pull request Oct 6, 2021
@andreasabel
Re haskell#7670: WIP: test case for haskell#7248
3c09d1b 
Two problems remaining:
- Warning output is not recorded in the golden value.
- The correct warning is not produced yet.

Current warning:

    Warning: No remote package servers have been specified. Usually you would have one specified in the config file.
@andreasabel andreasabel mentioned this pull request Oct 6, 2021
Merged
3 tasks
@andreasabel
Copy link
Member

@Mikolaj wrote:

Would somebody volunteer to add the simple test?

I gave it a try at #7702, but need some advice (see there).

fgaz
fgaz approved these changes Oct 6, 2021
View reviewed changes
Copy link
Member

@fgaz fgaz left a comment

Choose a reason for hiding this comment

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

Since we have a pr with a test, let's not delay this further. Thanks @ivanperez-keera and @andreasabel

@fgaz fgaz added the merge me Tell Mergify Bot to merge label Oct 6, 2021
@fgaz
Copy link
Member

fgaz commented Oct 6, 2021

@Mergifyio backport 3.6

@mergify
Copy link
Contributor

mergify bot commented Oct 6, 2021

Command backport 3.6: pending

backport 3.6 is pending

@ivanperez-keera
Remove debug info from end of error message (#7248)
04aab68 
The function that informs users that the package list is missing prints
an internal data structure at the end of the message, without formatting
it.

This extra info is likely just left over from debugging and does not
need to be printed.

This commit just removes that unnecessary show.
@mergify mergify bot merged commit bb08616 into haskell:master Oct 6, 2021
@mergify mergify bot mentioned this pull request Oct 6, 2021
Merged
@mergify
Copy link
Contributor

mergify bot commented Oct 6, 2021

Command backport 3.6: success

Backports have been created

andreasabel added a commit to andreasabel/cabal that referenced this pull request Oct 6, 2021
@andreasabel
Re haskell#7670: WIP: test case for haskell#7248
58a7d9b 
Two problems remaining:
- Warning output is not recorded in the golden value.
- The correct warning is not produced yet.

Current warning:

    Warning: No remote package servers have been specified. Usually you would have one specified in the config file.
andreasabel added a commit to andreasabel/cabal that referenced this pull request Oct 6, 2021
@andreasabel
Re haskell#7670: completed: test case for haskell#7248
b2cd2d4
andreasabel added a commit to andreasabel/cabal that referenced this pull request Oct 6, 2021
@andreasabel
Re haskell#7670: update test output witnessing the fix of haskell#7248
ffa806f
mergify bot added a commit that referenced this pull request Oct 7, 2021
@mergify
Merge pull request #7703 from haskell/mergify/bp/3.6/pr-7670
c0ea02b 
Remove debug info from end of an error message (#7248) (backport #7670)
andreasabel added a commit to andreasabel/cabal that referenced this pull request Oct 7, 2021
@andreasabel
Re haskell#7670: use cmd 'get' instead of 'unpack'
026f944
andreasabel added a commit to andreasabel/cabal that referenced this pull request Oct 7, 2021
@andreasabel
Re haskell#7670: rename test subdir 'Unpack' to 'Get'
97d710b
fgaz added a commit to andreasabel/cabal that referenced this pull request Oct 7, 2021
@fgaz
Re haskell#7670: use .invalid domain in test
5ba09c9
Mikolaj pushed a commit that referenced this pull request Oct 7, 2021
@andreasabel @fgaz
Re #7670: WIP: test case for #7248 (#7702)
6bb79dc 
* Re #7670: WIP: test case for #7248

Two problems remaining:
- Warning output is not recorded in the golden value.
- The correct warning is not produced yet.

Current warning:

    Warning: No remote package servers have been specified. Usually you would have one specified in the config file.

* Re #7670: completed: test case for #7248

* Re #7670: update test output witnessing the fix of #7248

* Re #7670: use cmd 'get' instead of 'unpack'

* Re #7670: rename test subdir 'Unpack' to 'Get'

* Re #7670: use .invalid domain in test

Co-authored-by: Francesco Gazzetta <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fgaz fgaz fgaz approved these changes

@andreasabel andreasabel andreasabel approved these changes

Assignees
No one assigned
Labels
attention: needs-review merge me Tell Mergify Bot to merge re: error-message Concerning error messages delivered to the user
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Error message about missing package list shows internal data structure RemoteRepo
4 participants
@ivanperez-keera @fgaz @Mikolaj @andreasabel