Skip to content

Classifies names according to case, uses for filename generation. #1764

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

Closed
wants to merge 7 commits into from

Conversation

gspencergoog
Copy link
Collaborator

@gspencergoog gspencergoog commented Sep 27, 2018

Currently, when you generate docs for dartdoc itself, there are some symbols (from the args package, but they could be from anywhere) where there are metamers of those symbols: When dartdoc generates the filenames, it generates the same name with different case. This is fine on Linux, where filenames are case sensitive, but on macOS and Windows, which are case preserving, but case insensitive, they map to the same file. So, once you check it in to git, you can't sync that repo to macOS or Windows without git complaining that one or the other of the files are modified.

This fixes that problem by classifying types as having different capitalization (camel case, upper case, lower case, lower camel case), and using those in the filenames. The default is CamelCase, so no identifier is added to symbols with camel case names, but others get -upper, -lower, -lowerCamel, or -camel, depending on their type. Each kind of symbol has a "case default" that is implied and not appended if the name matches that case. For instance, a class using camel case, or a method using lower camel case would not have any suffix. In addition, it will only add a suffix if there are metamers in the first place.

For cases that don't fall into those (for instance theUI and theUi are both lowerCamel, but they would have the same filename on a case-insensitive filesystem), it appends a set of numbers that represent the case runs (the number of consecutive letters with the same case) in the name (for instance theUi would have -lowerCamel-0311 appended, but theUI would have -lowerCamel-32 added to make them unique).

Addresses #1196

@googlebot googlebot added the cla: yes Google CLA check succeeded. label Sep 27, 2018
@gspencergoog gspencergoog force-pushed the case_fix branch 3 times, most recently from f456579 to acbcdab Compare September 27, 2018 19:37
!writtenFiles
.map((String file) => file.toLowerCase())
.contains(filePath.toLowerCase()),
'Filename case metamer found. Output would not be correct on a '
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 see where writtenFiles is being populated with lowercase names.

also nit: once it is, this case is actually the union of the previous case and the metamer case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's populated in the .map() in this expression. I can't just populate writtenFiles with lowercase names from the start, because on Linux it would be incorrect because it's case sensitive, and would be looking for the wrong file in the allowOverwrite case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, right. That's likely to be really slow, as we'll convert the entire set every time, but yes it will work.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I think I'll just make two lists (one actual case, and one lowercase) to make it more efficient.

@@ -73,7 +74,7 @@ <h5>ex library</h5>
<li><a href="ex/COLOR_GREEN-constant.html">COLOR_GREEN</a></li>
<li><a href="ex/COLOR_ORANGE-constant.html">COLOR_ORANGE</a></li>
<li><a href="ex/COMPLEX_COLOR-constant.html">COMPLEX_COLOR</a></li>
<li><a href="ex/deprecated-constant.html">deprecated</a></li>
<li><a href="ex/deprecated-lower-constant.html">deprecated</a></li>
Copy link
Contributor

Choose a reason for hiding this comment

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

What's going on here? I wouldn't have expected this one to shift.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think because constants have a default case of "UPPER", it names it this way. It shouldn't be doing it unless there is a conflict, however. I'll check into it.

.forEach((ModelElement e) => nameRegistry.add(e.library));
library._allModelElements
.forEach((ModelElement e) => library.nameRegistry.add(e));
library._allClasses?.forEach(collectClassNames);
Copy link
Contributor

Choose a reason for hiding this comment

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

What motivated this? IIUC, interfaces/mixins/supertypes and so on should be picked up by the libraries that contain them, and also not be relevant to the library containing the class they reference.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Huh. Yeah, I guess you're right. I had that in there before I was making sure to add all the names from the model elements in the library, so maybe that was just redundant.

}

nameRegistry.add(library);
library._allModelElements
Copy link
Contributor

Choose a reason for hiding this comment

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

I think only canonical ModelElements are likely to be relevant here, as those are the only ones we will ever generate links to. However, doing all ModelElements the same might simplify matters so OK to leave this if it does.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, this is literally all modelElements for the library, including nested members of classes, parameters, and so on that will never be written into the same directory. That might cause "collisions" that aren't real.

for (var metamer in metamers) {
String name = metamer.fullName.toLowerCase();
if (members.contains(name))
candidates.add(metamer);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: braces for multiline if

/// Builds a cache of unique names to speed up access to them once all the
/// names are collected. Only runs once, on the first access to a unique name
/// for this [NameRegistry].
void _buildCache() {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: typical dartdoc style is to build the cached item as part of the getter wrapping a variable. For private members, this can look something like:

Thingy __theThing;
Thingy get _theThing {
  if (__theThing == null) {
    _theThing = new Thingy(doSomeStuff());
  }
  return _theThing;
}

Not super strict on it, but it would be nice for readability to make this consistent.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can't quite get there, because it needs to supply an argument that isn't always the name. (a unique dirName for libraries is also found using this function).

@gspencergoog
Copy link
Collaborator Author

This was an ambitious project, and it almost works, but it's a year out of date now, and would probably be a nightmare to merge, so I'm going to abandon it for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Google CLA check succeeded.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants