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

refactor: further separate CLI logic from the API related functionality (see #117) #124

Open
wants to merge 15 commits into
base: v3
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -154,13 +154,14 @@ languagetool-rust = "^2.1"

```rust
use languagetool_rust::api::{check, server::ServerClient};
use std::borrow::Cow;

#[tokio::main]
async fn main() -> Result<(), Box<dyn std::error::Error>> {
let client = ServerClient::from_env_or_default();

let req = check::Request::default()
.with_text("Some phrase with a smal mistake".to_string()); // # codespell:ignore smal
.with_text(Cow::Borrowed("Some phrase with a smal mistake")); // # codespell:ignore smal

println!(
"{}",
Expand Down
8 changes: 5 additions & 3 deletions benches/benchmarks/check_texts.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use std::borrow::Cow;

use codspeed_criterion_compat::{criterion_group, Criterion, Throughput};
use futures::future::join_all;
use languagetool_rust::{
Expand Down Expand Up @@ -29,12 +31,12 @@ async fn request_until_success(req: &Request, client: &ServerClient) -> Response
}

#[tokio::main]
async fn check_text_basic(text: &str) -> Response {
async fn check_text_basic(text: &'static str) -> Response {
let client = ServerClient::from_env().expect(
"Please use a local server for benchmarking, and configure the environ variables to use \
it.",
);
let req = Request::default().with_text(text.to_string());
let req = Request::default().with_text(Cow::Borrowed(text));
request_until_success(&req, &client).await
}

Expand All @@ -48,7 +50,7 @@ async fn check_text_split(text: &str) -> Response {

let resps = join_all(lines.map(|line| {
async {
let req = Request::default().with_text(line.to_string());
let req = Request::default().with_text(Cow::Owned(line.to_string()));
let resp = request_until_success(&req, &client).await;
check::ResponseWithContext::new(req.get_text(), resp)
}
Expand Down
47 changes: 28 additions & 19 deletions src/api/check.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
//! Structures for `check` requests and responses.

use std::ops::Deref;
use std::{borrow::Cow, mem, ops::Deref};

#[cfg(feature = "annotate")]
use annotate_snippets::{
Expand Down Expand Up @@ -395,7 +395,7 @@ pub struct Request {
clap(short = 't', long, conflicts_with = "data", allow_hyphen_values(true))
)]
#[serde(skip_serializing_if = "Option::is_none")]
pub text: Option<String>,
pub text: Option<Cow<'static, str>>,
/// The text to be checked, given as a JSON document that specifies what's
/// text and what's markup. This or 'text' is required.
///
Expand Down Expand Up @@ -532,7 +532,7 @@ impl Default for Request {
impl Request {
/// Set the text to be checked and remove potential data field.
#[must_use]
pub fn with_text(mut self, text: String) -> Self {
pub fn with_text(mut self, text: Cow<'static, str>) -> Self {
self.text = Some(text);
self.data = None;
self
Expand Down Expand Up @@ -565,7 +565,7 @@ impl Request {
///
/// If both `self.text` and `self.data` are [`None`].
/// If any data annotation does not contain text or markup.
pub fn try_get_text(&self) -> Result<String> {
pub fn try_get_text(&self) -> Result<Cow<'static, str>> {
if let Some(ref text) = self.text {
Ok(text.clone())
} else if let Some(ref data) = self.data {
Expand All @@ -581,7 +581,7 @@ impl Request {
));
}
}
Ok(text)
Ok(Cow::Owned(text))
} else {
Err(Error::InvalidRequest(
"missing either text or data field".to_string(),
Expand All @@ -597,7 +597,7 @@ impl Request {
/// If both `self.text` and `self.data` are [`None`].
/// If any data annotation does not contain text or markup.
#[must_use]
pub fn get_text(&self) -> String {
pub fn get_text(&self) -> Cow<'static, str> {
self.try_get_text().unwrap()
}

Expand All @@ -607,15 +607,20 @@ impl Request {
/// # Errors
///
/// If `self.text` is none.
pub fn try_split(&self, n: usize, pat: &str) -> Result<Vec<Self>> {
let text = self
.text
.as_ref()
.ok_or(Error::InvalidRequest("missing text field".to_string()))?;
pub fn try_split(mut self, n: usize, pat: &str) -> Result<Vec<Self>> {
let text = mem::take(&mut self.text)
Copy link
Owner

Choose a reason for hiding this comment

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

Here, one goal would be that try_split returns a reference to &self, so the signature should be as follows:

pub fn try_split<'source>(&'source self, n: usize, pat: &str) -> Result<Vec<Self<'source>>>

Note that we should not need to mutate the actual request, because we only take references.

.ok_or_else(|| Error::InvalidRequest("missing text field".to_string()))?;
let string: &str = match &text {
Cow::Owned(s) => s.as_str(),
Cow::Borrowed(s) => s,
};

Ok(split_len(text.as_str(), n, pat)
Ok(split_len(string, n, pat)
.iter()
.map(|text_fragment| self.clone().with_text(text_fragment.to_string()))
.map(|text_fragment| {
self.clone()
.with_text(Cow::Owned(text_fragment.to_string()))
})
.collect())
}

Expand All @@ -627,7 +632,7 @@ impl Request {
///
/// If `self.text` is none.
#[must_use]
pub fn split(&self, n: usize, pat: &str) -> Vec<Self> {
pub fn split(self, n: usize, pat: &str) -> Vec<Self> {
self.try_split(n, pat).unwrap()
}
}
Expand All @@ -639,15 +644,15 @@ mod request_tests {

#[test]
fn test_with_text() {
let req = Request::default().with_text("hello".to_string());
let req = Request::default().with_text(std::borrow::Cow::Borrowed("hello"));

assert_eq!(req.text.unwrap(), "hello".to_string());
assert!(req.data.is_none());
}

#[test]
fn test_with_data() {
let req = Request::default().with_text("hello".to_string());
let req = Request::default().with_text(std::borrow::Cow::Borrowed("hello"));

assert_eq!(req.text.unwrap(), "hello".to_string());
assert!(req.data.is_none());
Expand Down Expand Up @@ -953,7 +958,7 @@ impl Response {
#[derive(Debug, Clone, PartialEq)]
pub struct ResponseWithContext {
/// Original text that was checked by LT.
pub text: String,
pub text: Cow<'static, str>,
/// Check response.
pub response: Response,
/// Text's length.
Expand All @@ -970,7 +975,7 @@ impl Deref for ResponseWithContext {
impl ResponseWithContext {
/// Bind a check response with its original text.
#[must_use]
pub fn new(text: String, response: Response) -> Self {
pub fn new(text: Cow<'static, str>, response: Response) -> Self {
let text_length = text.chars().count();
Self {
text,
Expand Down Expand Up @@ -1023,8 +1028,12 @@ impl ResponseWithContext {
}

self.response.matches.append(&mut other.response.matches);
self.text.push_str(other.text.as_str());

let mut string = self.text.into_owned();
string.push_str(other.text.as_ref());
self.text = Cow::Owned(string);
self.text_length += other.text_length;

self
}
}
Expand Down
10 changes: 7 additions & 3 deletions src/api/server.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
//! Structure to communicate with some `LanguageTool` server through the API.

use std::borrow::Cow;

use crate::{
api::{
check::{self, Request, Response},
Expand Down Expand Up @@ -422,7 +424,7 @@ impl ServerClient {
let text = request.text.ok_or(Error::InvalidRequest(
"missing text field; cannot join requests with data annotations".to_string(),
))?;
Result::<(String, Response)>::Ok((text, response))
Result::<(Cow<'static, str>, Response)>::Ok((text, response))
}));
}

Expand Down Expand Up @@ -456,7 +458,7 @@ impl ServerClient {
let text = request.get_text();
let resp = self.check(request).await?;

Ok(resp.annotate(text.as_str(), origin, color))
Ok(resp.annotate(text.as_ref(), origin, color))
}

/// Send a languages request to the server and await for the response.
Expand Down Expand Up @@ -587,6 +589,8 @@ impl ServerClient {

#[cfg(test)]
mod tests {
use std::borrow::Cow;

use super::ServerClient;
use crate::api::check::Request;

Expand All @@ -599,7 +603,7 @@ mod tests {
#[tokio::test]
async fn test_server_check_text() {
let client = ServerClient::from_env_or_default();
let req = Request::default().with_text("je suis une poupee".to_string());
let req = Request::default().with_text(Cow::Borrowed("je suis une poupee"));
assert!(client.check(&req).await.is_ok());
}

Expand Down
10 changes: 5 additions & 5 deletions src/cli/check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
//! - annotated data, if `--data TEXT` is provided;
//! - text from file(s), if `[FILE(S)]...` are provided.
//! - raw text through `stdin`, if nothing else is provided.
use std::{io::Write, path::PathBuf};
use std::{borrow::Cow, io::Write, path::PathBuf};

use clap::{Parser, ValueEnum};
use termcolor::{StandardStream, WriteColor};
Expand Down Expand Up @@ -87,7 +87,7 @@ impl ExecuteSubcommand for Command {
if request.text.is_none() && request.data.is_none() {
let mut text = String::new();
super::read_from_stdin(&mut stdout, &mut text)?;
request = request.with_text(text);
request = request.with_text(Cow::Owned(text));
}

if request.text.is_none() {
Expand All @@ -103,7 +103,7 @@ impl ExecuteSubcommand for Command {
writeln!(
&mut stdout,
"{}",
&response.annotate(response.text.as_str(), None, color)
&response.annotate(response.text.as_ref(), None, color)
)?;

return Ok(());
Expand All @@ -114,15 +114,15 @@ impl ExecuteSubcommand for Command {
let text = std::fs::read_to_string(filename)?;
let requests = request
.clone()
.with_text(text)
.with_text(Cow::Owned(text))
.split(self.max_length, self.split_pattern.as_str());
let response = server_client.check_multiple_and_join(requests).await?;

if !self.raw {
writeln!(
&mut stdout,
"{}",
&response.annotate(response.text.as_str(), filename.to_str(), color)
&response.annotate(response.text.as_ref(), filename.to_str(), color)
)?;
} else {
writeln!(&mut stdout, "{}", serde_json::to_string_pretty(&*response)?)?;
Expand Down
3 changes: 2 additions & 1 deletion tests/match_positions.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use languagetool_rust::api::{check, server::ServerClient};
use std::borrow::Cow;

#[macro_export]
macro_rules! test_match_positions {
Expand All @@ -7,7 +8,7 @@ macro_rules! test_match_positions {
async fn $name() -> Result<(), Box<dyn std::error::Error>> {

let client = ServerClient::from_env_or_default();
let req = check::Request::default().with_text($text.to_string());
let req = check::Request::default().with_text(Cow::Borrowed($text));
let resp = client.check(&req).await.unwrap();
let resp = check::ResponseWithContext::new(req.get_text(), resp);

Expand Down
Loading