Skip to content

Add more documentation #329

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

Merged
merged 2 commits into from
Jul 25, 2018
Merged

Add more documentation #329

merged 2 commits into from
Jul 25, 2018

Conversation

richard-uk1
Copy link

I recently submitted a PR to add documentation. This PR adds some more.

Note it won't merge as-is because it relies on PR #199 in servo/string-cache.

Copy link
Member

@jdm jdm left a comment

Choose a reason for hiding this comment

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

Good additions!

let local_names = Path::new(&env::var("CARGO_MANIFEST_DIR").unwrap()).join("local_names.txt");
let mut local_names_atom = string_cache_codegen::AtomType::new("LocalName", "local_name!");
for line in BufReader::new(File::open(&local_names).unwrap()).lines() {
let local_name = line.unwrap();
local_names_atom.atom(&local_name);
local_names_atom.atom(&local_name.to_ascii_lowercase());
}
local_names_atom.write_to(&mut generated).unwrap();
local_names_atom
.with_macro_doc("Takes a local name as a string and returns its key in the a string cache.")
Copy link
Member

Choose a reason for hiding this comment

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

in the a?

Copy link
Author

Choose a reason for hiding this comment

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

another oops!

.atoms(NAMESPACES.iter().map(|&(_prefix, url)| url))
.write_to(&mut generated)
.unwrap();

writeln!(generated, "#[macro_export] macro_rules! ns {{").unwrap();
// T
Copy link
Member

Choose a reason for hiding this comment

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

?

Copy link
Author

Choose a reason for hiding this comment

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

oops!

@@ -119,6 +144,7 @@ impl QualName {
}
}

/// Take a reference as an `ExpandedName`, dropping the unresolved prefix.
Copy link
Member

Choose a reason for hiding this comment

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

It's not clear what "Take a reference" means in this case.

Copy link
Author

Choose a reason for hiding this comment

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

I've changed this to "Take a reference of self". Do you think this is clear enough, or would you rather something else?

Copy link
Member

Choose a reason for hiding this comment

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

I think Return this qualified name as an ExpandedName without the unresolved prefix. is more clear.

Copy link
Author

Choose a reason for hiding this comment

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

Ok np. :)

//! See the [document object model article on wikipedia][dom wiki] for more information.
//!
//! This implementation stores the information associated with each node once, and then hands out
//! refs to children. The nodes themselves are reference-counted to avoid copy - you can create a
Copy link
Member

Choose a reason for hiding this comment

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

to avoid copying

@richard-uk1
Copy link
Author

This is blocked on a new release of string-cache-codegen and dependency bumps.

@bors-servo
Copy link
Contributor

☔ The latest upstream changes (presumably #332) made this pull request unmergeable. Please resolve the merge conflicts.

@Ygg01
Copy link
Contributor

Ygg01 commented Dec 12, 2017

@derekdreery Great work, but it seems some upstream changes, kinda broke it.

@richard-uk1
Copy link
Author

richard-uk1 commented Dec 12, 2017 via email

@richard-uk1
Copy link
Author

I'm trying to tidy up old pull requests. I've rebased off latest master (as of 2018-07-25). I think this is ready for review again.

@jdm
Copy link
Member

jdm commented Jul 25, 2018

@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit 0db8e21 has been approved by jdm

@bors-servo
Copy link
Contributor

⌛ Testing commit 0db8e21 with merge 24bd183...

bors-servo pushed a commit that referenced this pull request Jul 25, 2018
Add more documentation

I recently submitted a PR to add documentation. This PR adds some more.

Note it won't merge as-is because it relies on PR [#199 in servo/string-cache].

[#199 in servo/string-cache]: servo/string-cache#199
@bors-servo
Copy link
Contributor

☀️ Test successful - status-travis
Approved by: jdm
Pushing 24bd183 to master...

@bors-servo bors-servo merged commit 0db8e21 into servo:master Jul 25, 2018
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.

4 participants