After a few weeks of back and forth with @jcamiel, the main developer/collaborator of the Hurl project, I'm happy to report that, despite the two weeks being filled with challenging quests, things are going well!
#3481 - 🍺 The Preface
This is a prerequisite for the main feature, the --header option. In this PR, I created a helper method to aggregate a HeaderVec (a custom struct that contains a list of Header) with a list of raw headers (headers in plain text, for example: "User-Agent: hurl/6.1.0").
The method was straightforward. I simply cloned the headers inside the existing HeaderVec. Then I parsed the raw headers into instances of Header and pushed them into the clone. Finally, the method returns this clone.
To parse the raw headers, I played with different ideas and eventually discovered a shorthand: filter_map. This method simplifies a chain of .map(...).filter(...).map(...) calls into just one. It maps content according to the closure, and only retains those which returns a Option::Some(T). Then it unwraps the enum and returns the actual content - All within one method.
The PR also includes documentation right next to the method and a unit test, both of which I wrote mimicking the style of existing content.
#3498 - 📜 The Side-Quest
This is a simple change suggested by the maintainer while I was working on #3419 (see below). My original unit test doesn't cover any testing of repeated headers. The maintainer had some comments before regarding its behaviour, so it makes sense to take it into consideration.
Here I had an oversight: I initially only included repeated headers in the raw headers list. Later the maintainer reminded me to include one for the original HeaderVec as well. This way the aggregation method can get tested properly.
#3419 - ⚔️ The Great War
This is the main feature. I started this PR all the way back in November. However, the original PR didn't quite fall in line with the maintainer's expectation.
My initial implementation didn't take built-in headers into consideration at all, which could cause some serious unwanted behavior. This was also why the maintainer proposed #3481 - to aggregate the headers first and centralize the logic where they are handled.
The maintainer gave me some instructions in extreme details (again huge thanks to @jcamiel 🙏). I mostly followed them but did also run into some problems. Fortunately, I was able to solve them through discussions. There's one conversation I'd like to highlight:
&[&str] vs &[String]
Fighting with the borrow checker is just everyday housework for Rust developers, but this one turned into an interesting conversation.
In this project, headers, like all other command line options, are stores as Vec<String>. However, the maintainer's proposal for the aggregation method calls for a &[&str] as its parameter, which resulted in a type mismatch.
At this stage I didn't quite understand how slices work in Rust, so I did some research myself: Taking a slice as function parameter offers the flexibility that it can accept any list that implements the Deref trait. For example, when you pass a &Vec<String> to a function that takes &[String], the vector is automatically dereferenced into a slice.
The problem is, a slice of owned string ([String]) is not the same as a slice of string slices ([&str]), and there's no automatic conversion between them either. I proposed to simply change the parameter from &[&str] to &[String], since it matches the original data type. My idea was to simplify the code.
However, the maintainer argued that &[&str] is more idiomatic and possibly more memory efficient, to which I actually agreed - We don't really need the ownership of these strings, so taking owned strings doesn't make much sense. The only problem is that I needed to manually convert the headers from Vec<String> to Vec<&str>.
In this case, I don't think there's a correct answer. Both options have their pros and cons. We discussed the compromises of each option and I eventually decided to go with the maintainer's idea.
🏕️ Conclusion
Just as I predicted at the beginning, it's been a busy couple of weeks! And there's still more work to do: There's the request to implement the same header feature for the hurl file. There's also a new issue regarding the integration test on this feature, which I've already taken! Let's see how things go from here! 😆
Top comments (0)