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

Streamline the error handling in archive.ml #176

Merged
merged 1 commit into from
Nov 21, 2019
Merged

Streamline the error handling in archive.ml #176

merged 1 commit into from
Nov 21, 2019

Conversation

gpetiot
Copy link
Contributor

@gpetiot gpetiot commented Nov 15, 2019

  • replace the exceptions with error types in archive.ml
  • defining a Sbytes module to wrap 2 unsafe functions of Bytes
  • minor style changes (put the infix operator and the anonymous function argument at the end of the line) (applying ocamlformat would generate a huge diff for now)

Copy link
Contributor

@NathanReb NathanReb left a comment

Choose a reason for hiding this comment

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

Looks good! This is a welcome change, thanks!

I still have a few minor comments but nothing too serious!

@gpetiot gpetiot requested a review from NathanReb November 20, 2019 09:48
Copy link
Contributor

@NathanReb NathanReb left a comment

Choose a reason for hiding this comment

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

Looks good!

Can you please fix the changelog thing and then let's merge!

CHANGES.md Outdated
@@ -12,6 +12,9 @@

### Security

- Use error types instead of relying on exceptions in buffer operations
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it should be in Security as I would reserve this for serious issues such as leaking passwords or cryptographic keys.

If these errors could leak to users I'd say it should be in Fixed with a message that speaks to users, something like:
Fix uncaught exceptions in distrib subcommand and replace them with proper error messages.

Otherwise it probably shouldn't even be in the changelog!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@NathanReb NathanReb merged commit 2d61dbf into tarides:master Nov 21, 2019
pitag-ha added a commit to pitag-ha/opam-repository that referenced this pull request Jul 13, 2020
CHANGES:

### Added

- Add a `dune-release config` subcommand to display and edit the global
  configuration (tarides/dune-release#220, @NathanReb).
- Add command `delegate-info` to print information needed by external
  release scripts (tarides/dune-release#221, @pitag-ha)
- Use Curly instead of Cmd to interact with github (tarides/dune-release#202, @gpetiot)
- Add `x-commit-hash` field to the opam file when releasing (tarides/dune-release#224, @gpetiot)
- Add support for common alternative names for the license and
  ChangeLog file (tarides/dune-release#204, @paurkedal)

### Changed

- Command `tag`: improve error and log messages by comparing the provided
  commit with the commit correspondent to the provided tag (tarides/dune-release#226, @pitag-ha)
- Error logs: when an external command fails, include its error message in
  the error message posted by `dune-release` (tarides/dune-release#231, @pitag-ha)
- Error log formatting: avoid unnecessary line-breaks; indent only slightly
  in multi-lines (tarides/dune-release#234, @pitag-ha)
- Linting step of `dune-release distrib` does not fail when opam's `doc` field
  is missing. Do not try to generate nor publish the documentation when opam's
  `doc` field is missing. (tarides/dune-release#235, @gpetiot)

### Deprecated

- Deprecate opam 1.x (tarides/dune-release#195, @gpetiot)

### Fixed

- Separate packages names by spaces in `publish` logs (tarides/dune-release#171, @hannesm)
- Fix uncaught exceptions in distrib subcommand and replace them with proper
  error messages (tarides/dune-release#176, @gpetiot)
- Use the 'user' field in the configuration before inferring it from repo URI
  and handles HTTPS URIs (tarides/dune-release#183, @gpetiot)
- Ignore backup files when looking for README, CHANGES and LICENSE files
  (tarides/dune-release#194, @gpetiot)
- Do not echo input characters when reading token (tarides/dune-release#199, @gpetiot)
- Improve the output of VCS command errors (tarides/dune-release#193, @gpetiot)
- Better error handling when checking opam version (tarides/dune-release#195, @gpetiot)
- Do not write 'version' and 'name' fields in opam file (tarides/dune-release#200, @gpetiot)
- Use Yojson to parse github json response and avoid parsing bugs.
  (tarides/dune-release#177, @gpetiot)
- The `git` command used in `publish doc` should check `DUNE_RELEASE_GIT` (even
  if deprecated) before `PATH`. (tarides/dune-release#242, @gpetiot)
- Adapt the docs to the removal of the `log` subcommand (tarides/dune-release#196, @gpetiot)

### Removed

### Security
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